Re: Review Request 46323: Propagated KillPolicy in kill task from scheduler to executor.

2016-05-10 Thread Alexander Rukletsov 

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

(Updated May 10, 2016, 1:28 p.m.)


Review request for mesos and Ben Mahler.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/internal/evolve.hpp cff6b657a6d69ff9e7f7dedc965122a03a200da1 
  src/internal/evolve.cpp 4ca2abacd75523de8fb4ffe69a9edd8627f512af 
  src/master/master.cpp bcc3b5bf698457f8baa9a8b2633a7e76c304da55 
  src/slave/slave.hpp b72438033708de473046d321c493d9fbcd7a9b43 
  src/slave/slave.cpp 116ea59b72950db4a7cd721b7ba5bfbb2e1c1454 
  src/tests/mesos.hpp e8df86a361f18f174dd38c65df08ee3215091800 
  src/tests/mesos.cpp 036c589f5aafc8c804b0fb4e5ad62df70e471e88 
  src/tests/slave_tests.cpp 3f653354869987dce3f5fbc4513b6f3466a718cb 

Diff: https://reviews.apache.org/r/46323/diff/


Testing
---

The whole chain is tested in https://reviews.apache.org/r/46325/


Thanks,

Alexander Rukletsov

Re: Review Request 46323: Propagated KillPolicy in kill task from scheduler to executor.

2016-05-05 Thread Alexander Rukletsov 

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

(Updated May 5, 2016, 3:38 p.m.)


Review request for mesos and Ben Mahler.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/internal/evolve.hpp cff6b657a6d69ff9e7f7dedc965122a03a200da1 
  src/internal/evolve.cpp 4ca2abacd75523de8fb4ffe69a9edd8627f512af 
  src/master/master.cpp 6d3e0f7c634690a35eec1ce827b705e04c3af87e 
  src/slave/slave.hpp b72438033708de473046d321c493d9fbcd7a9b43 
  src/slave/slave.cpp 116ea59b72950db4a7cd721b7ba5bfbb2e1c1454 
  src/tests/mesos.hpp 0f6f541c5d2007a69ad5bd6e884235cd3c0c1be2 
  src/tests/mesos.cpp 036c589f5aafc8c804b0fb4e5ad62df70e471e88 
  src/tests/slave_tests.cpp 3f653354869987dce3f5fbc4513b6f3466a718cb 

Diff: https://reviews.apache.org/r/46323/diff/


Testing
---

The whole chain is tested in https://reviews.apache.org/r/46325/


Thanks,

Alexander Rukletsov

Re: Review Request 46323: Propagated KillPolicy in kill task from scheduler to executor.

2016-04-22 Thread Alexander Rukletsov 

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

(Updated April 22, 2016, 2:38 p.m.)


Review request for mesos and Ben Mahler.


Changes
---

BenM's comment.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/internal/evolve.hpp cff6b657a6d69ff9e7f7dedc965122a03a200da1 
  src/internal/evolve.cpp 4ca2abacd75523de8fb4ffe69a9edd8627f512af 
  src/master/master.cpp 6dacf5fbd73771e5c31ffb0e8723cd2905dcefb3 
  src/slave/slave.hpp 20a4bcd0bb9dad06ea81fc4ad9b2fa462c69d2c5 
  src/slave/slave.cpp a365e8f5f8d63bf80614fccf6adf35a4fee07526 
  src/tests/mesos.hpp e4b63d41d883807ac39846799468b80e88c84e0b 
  src/tests/mesos.cpp b5937af7713e1ee2af475518b3e968b2defa8beb 
  src/tests/slave_tests.cpp 3f653354869987dce3f5fbc4513b6f3466a718cb 

Diff: https://reviews.apache.org/r/46323/diff/


Testing
---

The whole chain is tested in https://reviews.apache.org/r/46325/


Thanks,

Alexander Rukletsov

Re: Review Request 46323: Propagated KillPolicy in kill task from scheduler to executor.

2016-04-21 Thread Ben Mahler 

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


Fix it, then Ship it!





src/slave/slave.cpp (lines 2041 - 2042)


const & for both of these since the rhs is not a temporary


- Ben Mahler


On April 21, 2016, 2:28 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46323/
> ---
> 
> (Updated April 21, 2016, 2:28 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-4908
> https://issues.apache.org/jira/browse/MESOS-4908
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/internal/evolve.hpp cff6b657a6d69ff9e7f7dedc965122a03a200da1 
>   src/internal/evolve.cpp 4ca2abacd75523de8fb4ffe69a9edd8627f512af 
>   src/master/master.cpp 6dacf5fbd73771e5c31ffb0e8723cd2905dcefb3 
>   src/slave/slave.hpp f78c1b4e4d5378ef7223c6eb3ea45491c30fb4c1 
>   src/slave/slave.cpp d82dec2b10d496065013eb4ad6a35dc054b72553 
>   src/tests/mesos.hpp e4b63d41d883807ac39846799468b80e88c84e0b 
>   src/tests/mesos.cpp b5937af7713e1ee2af475518b3e968b2defa8beb 
>   src/tests/slave_tests.cpp ee58488b0b927c7c5833add4718941539663e6d2 
> 
> Diff: https://reviews.apache.org/r/46323/diff/
> 
> 
> Testing
> ---
> 
> The whole chain is tested in https://reviews.apache.org/r/46325/
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 46323: Propagated KillPolicy in kill task from scheduler to executor.

2016-04-21 Thread Alexander Rukletsov 

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

(Updated April 21, 2016, 2:28 p.m.)


Review request for mesos and Ben Mahler.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/internal/evolve.hpp cff6b657a6d69ff9e7f7dedc965122a03a200da1 
  src/internal/evolve.cpp 4ca2abacd75523de8fb4ffe69a9edd8627f512af 
  src/master/master.cpp 6dacf5fbd73771e5c31ffb0e8723cd2905dcefb3 
  src/slave/slave.hpp f78c1b4e4d5378ef7223c6eb3ea45491c30fb4c1 
  src/slave/slave.cpp d82dec2b10d496065013eb4ad6a35dc054b72553 
  src/tests/mesos.hpp e4b63d41d883807ac39846799468b80e88c84e0b 
  src/tests/mesos.cpp b5937af7713e1ee2af475518b3e968b2defa8beb 
  src/tests/slave_tests.cpp ee58488b0b927c7c5833add4718941539663e6d2 

Diff: https://reviews.apache.org/r/46323/diff/


Testing
---

The whole chain is tested in https://reviews.apache.org/r/46325/


Thanks,

Alexander Rukletsov

Re: Review Request 46323: Propagated KillPolicy in kill task from scheduler to executor.

2016-04-21 Thread Alexander Rukletsov 

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

(Updated April 21, 2016, 2:06 p.m.)


Review request for mesos and Ben Mahler.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/internal/evolve.hpp cff6b657a6d69ff9e7f7dedc965122a03a200da1 
  src/internal/evolve.cpp 4ca2abacd75523de8fb4ffe69a9edd8627f512af 
  src/master/master.cpp 6dacf5fbd73771e5c31ffb0e8723cd2905dcefb3 
  src/slave/slave.hpp f78c1b4e4d5378ef7223c6eb3ea45491c30fb4c1 
  src/slave/slave.cpp d82dec2b10d496065013eb4ad6a35dc054b72553 
  src/tests/mesos.hpp e4b63d41d883807ac39846799468b80e88c84e0b 
  src/tests/mesos.cpp b5937af7713e1ee2af475518b3e968b2defa8beb 
  src/tests/slave_tests.cpp ee58488b0b927c7c5833add4718941539663e6d2 

Diff: https://reviews.apache.org/r/46323/diff/


Testing
---

The whole chain is tested in https://reviews.apache.org/r/46325/


Thanks,

Alexander Rukletsov

Re: Review Request 46323: Propagated KillPolicy in kill task from scheduler to executor.

2016-04-19 Thread Alexander Rukletsov 


> On April 19, 2016, 1:09 a.m., Ben Mahler wrote:
> > src/internal/evolve.hpp, line 46
> > 
> >
> > Why did you choose to inject it here? Seems better closer to TaskInfo?

Because in "mesos.proto" `KillPolicy` is defined between `FrameworkInfo` and 
`ExecutorInfo`. Do you think we should move it closer to `TaskInfo` in *both* 
"mesos.proto" and "evolve.hpp"?


> On April 19, 2016, 1:09 a.m., Ben Mahler wrote:
> > src/master/master.cpp, lines 4060-4061
> > 
> >
> > Since it seems straightforward, why not just handle the field instead 
> > of introducing this NOTE? It seems nice to add the straightforward support 
> > here and decide on the old API later (if at all).

We can do it, but I don't understand the purpose (and I'm reluctant to add any 
code if there is no purpose). The scheduler driver uses `Call` message instead 
of internal `KillTaskMessage` since Mesos 0.24. The only use case that comes to 
my mind is pure bindings that do not use the scheduler driver and send 
`KillTaskMessage` directly. Do we want to add support for this case?


> On April 19, 2016, 1:09 a.m., Ben Mahler wrote:
> > src/slave/slave.hpp, line 149
> > 
> >
> > Converting an optional field into an Option is not yet supported, 
> > unless I'm missing something?
> > 
> > See: https://issues.apache.org/jira/browse/MESOS-2638
> > 
> > Note that I added the current workaround to that ticket description, 
> > you can take the entire message in the handler to check the optionality.

Hm, I see. I've noticed a similar pattern for 
https://github.com/apache/mesos/blob/45d5fc379ff59c537ffc9ddfdf33c845af1e381f/src/slave/slave.cpp#L683
 and thought we support that.


- Alexander


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


On April 18, 2016, 12:44 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46323/
> ---
> 
> (Updated April 18, 2016, 12:44 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-4908
> https://issues.apache.org/jira/browse/MESOS-4908
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/internal/evolve.hpp cff6b657a6d69ff9e7f7dedc965122a03a200da1 
>   src/internal/evolve.cpp 4ca2abacd75523de8fb4ffe69a9edd8627f512af 
>   src/master/master.cpp 6dacf5fbd73771e5c31ffb0e8723cd2905dcefb3 
>   src/slave/slave.hpp f78c1b4e4d5378ef7223c6eb3ea45491c30fb4c1 
>   src/slave/slave.cpp d82dec2b10d496065013eb4ad6a35dc054b72553 
>   src/tests/mesos.hpp e4b63d41d883807ac39846799468b80e88c84e0b 
>   src/tests/mesos.cpp b5937af7713e1ee2af475518b3e968b2defa8beb 
>   src/tests/slave_tests.cpp ee58488b0b927c7c5833add4718941539663e6d2 
> 
> Diff: https://reviews.apache.org/r/46323/diff/
> 
> 
> Testing
> ---
> 
> The whole chain is tested in https://reviews.apache.org/r/46325/
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 46323: Propagated KillPolicy in kill task from scheduler to executor.

2016-04-18 Thread Ben Mahler 

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



Looks good, but held off on a ship it because there is a bug in the agent's 
message handler, see below.


src/internal/evolve.hpp (line 46)


Why did you choose to inject it here? Seems better closer to TaskInfo?



src/master/master.cpp (lines 4060 - 4061)


Since it seems straightforward, why not just handle the field instead of 
introducing this NOTE? It seems nice to add the straightforward support here 
and decide on the old API later (if at all).



src/slave/slave.hpp (line 149)


Converting an optional field into an Option is not yet supported, unless 
I'm missing something?

See: https://issues.apache.org/jira/browse/MESOS-2638

Note that I added the current workaround to that ticket description, you 
can take the entire message in the handler to check the optionality.


- Ben Mahler


On April 18, 2016, 12:44 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46323/
> ---
> 
> (Updated April 18, 2016, 12:44 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-4908
> https://issues.apache.org/jira/browse/MESOS-4908
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/internal/evolve.hpp cff6b657a6d69ff9e7f7dedc965122a03a200da1 
>   src/internal/evolve.cpp 4ca2abacd75523de8fb4ffe69a9edd8627f512af 
>   src/master/master.cpp 6dacf5fbd73771e5c31ffb0e8723cd2905dcefb3 
>   src/slave/slave.hpp f78c1b4e4d5378ef7223c6eb3ea45491c30fb4c1 
>   src/slave/slave.cpp d82dec2b10d496065013eb4ad6a35dc054b72553 
>   src/tests/mesos.hpp e4b63d41d883807ac39846799468b80e88c84e0b 
>   src/tests/mesos.cpp b5937af7713e1ee2af475518b3e968b2defa8beb 
>   src/tests/slave_tests.cpp ee58488b0b927c7c5833add4718941539663e6d2 
> 
> Diff: https://reviews.apache.org/r/46323/diff/
> 
> 
> Testing
> ---
> 
> The whole chain is tested in https://reviews.apache.org/r/46325/
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 46323: Propagated KillPolicy in kill task from scheduler to executor.

2016-04-18 Thread Alexander Rukletsov 

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

(Updated April 18, 2016, 12:44 p.m.)


Review request for mesos and Ben Mahler.


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


Repository: mesos


Description (updated)
---

See summary.


Diffs
-

  src/internal/evolve.hpp cff6b657a6d69ff9e7f7dedc965122a03a200da1 
  src/internal/evolve.cpp 4ca2abacd75523de8fb4ffe69a9edd8627f512af 
  src/master/master.cpp 6dacf5fbd73771e5c31ffb0e8723cd2905dcefb3 
  src/slave/slave.hpp f78c1b4e4d5378ef7223c6eb3ea45491c30fb4c1 
  src/slave/slave.cpp d82dec2b10d496065013eb4ad6a35dc054b72553 
  src/tests/mesos.hpp e4b63d41d883807ac39846799468b80e88c84e0b 
  src/tests/mesos.cpp b5937af7713e1ee2af475518b3e968b2defa8beb 
  src/tests/slave_tests.cpp ee58488b0b927c7c5833add4718941539663e6d2 

Diff: https://reviews.apache.org/r/46323/diff/


Testing
---

The whole chain is tested in https://reviews.apache.org/r/46325/


Thanks,

Alexander Rukletsov