mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Mahler <bmah...@apache.org>
Subject Review Request 65106: Removed the misleading `isRemovable` helper in the master.
Date Fri, 12 Jan 2018 01:34:39 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65106/
-----------------------------------------------------------

Review request for mesos, Vinod Kone and Jiang Yan Xu.


Bugs: MESOS-8389
    https://issues.apache.org/jira/browse/MESOS-8389


Repository: mesos


Description
-------

In the past, the notion of a "removable" task meant: the task is
terminal and acknowledged. However, the `isRemovable` helper only
defines removability by the task state (terminal or unreachable)
but not whether the terminal update is acknowledged.

As a result, the code that is calling `isRemovable` is unintuitive.
One example of a confusing piece of code is within updateTask. Here,
we have logic which says, if the task is removable, recover the
resources but don't remove it. This seems more intuitive if directly
described as: "if the task is no longer consuming resources (e.g.
transitioned to terminal or unreachable) then recover the resources".

If one looks up the documentation of `isRemovable`, it says "When a
task becomes removable, it is erased from the master's primary task
data structures", but that isn't accurate since this function doesn't
say whether the terminal task has been acknowledged, which is
required for a task to be removable.

To make an immediate improvement, this patch removes `isRemovable`
(no pun intended) in favor of directly checking the states we care
about. In the future, we may want to introduce some helpers like
`isAllocatedResources` to describe whether the task's resources are
considered allocated.

If we do introduce a notion of `isRemovable` in the future, it seems
it should take the `Task*` so that it can say whether the task could
be "removed" from the master, which includes checking that terminal
tasks have been acknowledged. However, "remove" is somewhat of a
confusing term, since what we're actually doing is "completing" the
task.


Diffs
-----

  src/master/master.hpp cdfd06ceb2a8eafa60d0af382b628b165e4ddcb5 
  src/master/master.cpp 7ed15e4ba2a31c5fe4b8571f645cdca69a3e82f4 


Diff: https://reviews.apache.org/r/65106/diff/1/


Testing
-------

make check


Thanks,

Benjamin Mahler


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message