-----------------------------------------------------------
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)
<https://reviews.apache.org/r/46323/#comment192878>

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



src/master/master.cpp (lines 4060 - 4061)
<https://reviews.apache.org/r/46323/#comment192888>

    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)
<https://reviews.apache.org/r/46323/#comment192881>

    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
> 
>

Reply via email to