Re: Review Request 65106: Removed the misleading `isRemovable` helper in the master.

2018-01-22 Thread Benjamin Mahler 


> On Jan. 23, 2018, 1:29 a.m., Vinod Kone wrote:
> > src/master/master.cpp
> > Lines 10170 (patched)
> > 
> >
> > s/Indicated/Indicates/ ?

Oh, yeah this isn't my comment but I'll touch it up here since it's been moved.


- Benjamin


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


On Jan. 23, 2018, 12:11 a.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65106/
> ---
> 
> (Updated Jan. 23, 2018, 12:11 a.m.)
> 
> 
> Review request for mesos, James Peach, 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 05136781b6b1539f37c283e8127e4bafb187a0d1 
>   src/master/master.cpp 3af96b1d2024ab1b951537ebc6bbc225cfa9cc88 
> 
> 
> Diff: https://reviews.apache.org/r/65106/diff/3/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 65106: Removed the misleading `isRemovable` helper in the master.

2018-01-22 Thread Vinod Kone 

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


Fix it, then Ship it!





src/master/master.cpp
Lines 10170 (patched)


s/Indicated/Indicates/ ?



src/master/master.cpp
Lines 10301 (patched)


s/simply/simplify/


- Vinod Kone


On Jan. 23, 2018, 12:11 a.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65106/
> ---
> 
> (Updated Jan. 23, 2018, 12:11 a.m.)
> 
> 
> Review request for mesos, James Peach, 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 05136781b6b1539f37c283e8127e4bafb187a0d1 
>   src/master/master.cpp 3af96b1d2024ab1b951537ebc6bbc225cfa9cc88 
> 
> 
> Diff: https://reviews.apache.org/r/65106/diff/3/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 65106: Removed the misleading `isRemovable` helper in the master.

2018-01-22 Thread Benjamin Mahler 


> On Jan. 22, 2018, 8:02 p.m., Vinod Kone wrote:
> >

Thank you for the review! :)


> On Jan. 22, 2018, 8:02 p.m., Vinod Kone wrote:
> > src/master/master.hpp
> > Lines 2208 (patched)
> > 
> >
> > can you use `CHECK_NE` here so that `task->state()` is auto printed 
> > when this fails?

Ditto cases below.


> On Jan. 22, 2018, 8:02 p.m., Vinod Kone wrote:
> > src/master/master.hpp
> > Lines 2334 (patched)
> > 
> >
> > ditto. `CHECK_NE`?

See my comment below, I avoided it because the CHECK_NE will print the state 
but we don't really need that here. Also added logging of the task/framework id.


> On Jan. 22, 2018, 8:02 p.m., Vinod Kone wrote:
> > src/master/master.cpp
> > Lines 10111-10127 (original), 10111-10119 (patched)
> > 
> >
> > move this to #1061
> > 
> > Also, consider moving this to `protobuf_utils.hpp`. AFAICT this is used 
> > in 4 different places in this diff.

This needs to be up here since we mutate the task state.

I looked into it originally, but when I was writing its documentation it seemed 
strange to need a helper for an or condition and it seemed like it only had 
some meaning according to the master's current semantics around resource 
recovery. So I held off.


> On Jan. 22, 2018, 8:02 p.m., Vinod Kone wrote:
> > src/master/master.cpp
> > Lines 10247-10261 (original), 10245-10261 (patched)
> > 
> >
> > Not yours, but I wish we can remove this if altogether and CHECK that 
> > we are only removing a terminal or unreachable task period. AFAICT, the 
> > only place where we violate this invariant is in `Master::finalize()`. 
> > Maybe add a TODO to fix?

Yeah I was looking to track this down and CHECK here instead, and ended up 
writing a TODO that captures why I couldn't, somehow I had removed that prior 
to publishing the patch. Added one.


> On Jan. 22, 2018, 8:02 p.m., Vinod Kone wrote:
> > src/master/master.cpp
> > Line 11423 (original), 11423 (patched)
> > 
> >
> > CHECK_NE

I avoided it here, since there wasn't a need to print the task state, if this 
fails it's TASK_UNREACHABLE. It seemed a little more readable to see the != 
operator.

Added the task id and framework id here for debuggability.


- Benjamin


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


On Jan. 23, 2018, 12:11 a.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65106/
> ---
> 
> (Updated Jan. 23, 2018, 12:11 a.m.)
> 
> 
> Review request for mesos, James Peach, 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

Re: Review Request 65106: Removed the misleading `isRemovable` helper in the master.

2018-01-22 Thread Benjamin Mahler 

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

(Updated Jan. 23, 2018, 12:11 a.m.)


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


Changes
---

Updated per vinod's review.


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 (updated)
-

  src/master/master.hpp 05136781b6b1539f37c283e8127e4bafb187a0d1 
  src/master/master.cpp 3af96b1d2024ab1b951537ebc6bbc225cfa9cc88 


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

Changes: https://reviews.apache.org/r/65106/diff/2-3/


Testing
---

make check


Thanks,

Benjamin Mahler

Re: Review Request 65106: Removed the misleading `isRemovable` helper in the master.

2018-01-22 Thread Vinod Kone 

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




src/master/master.hpp
Lines 2208 (patched)


can you use `CHECK_NE` here so that `task->state()` is auto printed when 
this fails?



src/master/master.hpp
Lines 2215 (patched)


s/the UI/in the UI/



src/master/master.hpp
Lines 2334 (patched)


ditto. `CHECK_NE`?



src/master/master.cpp
Lines 10111-10127 (original), 10111-10119 (patched)


move this to #1061

Also, consider moving this to `protobuf_utils.hpp`. AFAICT this is used in 
4 different places in this diff.



src/master/master.cpp
Lines 10247-10261 (original), 10245-10261 (patched)


Not yours, but I wish we can remove this if altogether and CHECK that we 
are only removing a terminal or unreachable task period. AFAICT, the only place 
where we violate this invariant is in `Master::finalize()`. Maybe add a TODO to 
fix?



src/master/master.cpp
Line 11423 (original), 11423 (patched)


CHECK_NE



src/master/master.cpp
Lines 11447 (patched)


Can you include taskid and frameworkid in the error message here for easy 
debuggability?


- Vinod Kone


On Jan. 12, 2018, 1:34 a.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65106/
> ---
> 
> (Updated Jan. 12, 2018, 1:34 a.m.)
> 
> 
> Review request for mesos, James Peach, 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 3d5180b8028b2a15ad111f91863e77747b362593 
>   src/master/master.cpp 465336d33008a848df063d4295416eb91f7bb44f 
> 
> 
> Diff: https://reviews.apache.org/r/65106/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 65106: Removed the misleading `isRemovable` helper in the master.

2018-01-12 Thread Benjamin Mahler 

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



Will need to rebase off of https://reviews.apache.org/r/64940/ once that's 
committed.

- Benjamin Mahler


On Jan. 12, 2018, 1:34 a.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65106/
> ---
> 
> (Updated Jan. 12, 2018, 1:34 a.m.)
> 
> 
> 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
> 
>