mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Alexander Rukletsov <ruklet...@gmail.com>
Subject Re: Review Request 46323: Propagated KillPolicy in kill task from scheduler to executor.
Date Tue, 19 Apr 2016 17:08:30 GMT


> On April 19, 2016, 1:09 a.m., Ben Mahler wrote:
> > src/internal/evolve.hpp, line 46
> > <https://reviews.apache.org/r/46323/diff/1/?file=1348360#file1348360line46>
> >
> >     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
> > <https://reviews.apache.org/r/46323/diff/1/?file=1348362#file1348362line4060>
> >
> >     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
> > <https://reviews.apache.org/r/46323/diff/1/?file=1348363#file1348363line149>
> >
> >     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
> 
>


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