mesos-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Ben Mahler" <benjamin.mah...@gmail.com>
Subject Re: Review Request 26701: Updated master to update task unacknowledged state properly.
Date Sat, 18 Oct 2014 23:15:52 GMT

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

Ship it!


Code looks good, thanks! Mostly left comments around better comments, and some minor code
movement.

Curious if we can pick a better name for `unacknowledged_state` in `Task`:

`status_update_state`
`status_update_uuid`

`current_status_update_state`
`current_status_update_uuid`

`latest_status_update_state`
`latest_status_update_uuid`


src/master/master.cpp
<https://reviews.apache.org/r/26701/#comment97845>

    Isn't there a bit more to this comment?
    
    If this is a 0.21.0 master and there are 0.20.0 slaves, this all makes sense (the 0.20.0
slaves will not send this field when re-registering so we won't have it here as you said).
    
    However, if this is a 0.21.0 master and there are 0.21.0 slaves, then the state would
have been set when the slave re-registered with the `Task`, right..?
    
    We never unset the unacknowledged state when an acknowledgement occurs on the slave either.
"unacknowledged" now seems a bit confusing.
    
    Let's chat on Monday so I can understand this a bit better :)



src/master/master.cpp
<https://reviews.apache.org/r/26701/#comment97846>

    Hopefully in clarifying my comment above we can be a little more specific in this warning
message.



src/master/master.cpp
<https://reviews.apache.org/r/26701/#comment97847>

    Could have the following above:
    
    // Must be set or unset together.
    CHECK(task->has_unacknoweldged_uuid() == task->has_unacknowledged_state());



src/master/master.cpp
<https://reviews.apache.org/r/26701/#comment97848>

    Maybe warrants a comment:
    
    // We remove the task once a terminal update has
    // been acknowledged by the framework.



src/master/master.cpp
<https://reviews.apache.org/r/26701/#comment97850>

    Why not move this down to where it's used?



src/master/master.cpp
<https://reviews.apache.org/r/26701/#comment97849>

    I'm a bit worried about this TODO because it's too inquisitive, someone coming along here
doesn't have any context and might think we can remove this.
    
    It should not be possible, but to protect against bugs here a CHECK is probably too severe,
which means we'll continue to log an error and guard it, in which case we should just update
the comment as a NOTE about this.
    
    Are you thinking of other alternatives?



src/master/master.cpp
<https://reviews.apache.org/r/26701/#comment97851>

    Can you set the state in the similar if / else above, any reason to do the same check
twice?



src/master/master.cpp
<https://reviews.apache.org/r/26701/#comment97852>

    Seems nice to move this up as well (when you move `set_state` up).



src/master/master.cpp
<https://reviews.apache.org/r/26701/#comment97853>

    Looks like this can be wrapped at the LOG(INFO) <<  line while still being under
80 characters:
    
    len('  LOG(INFO) << "Updating the latest state of task " << task->task_id()')
    len('            << " of framework " << task->framework_id()')
    len('            << " to " << task->state()')
    len('            << (task->state() != status.state()')
    len('                ? " (unacknowledged state: " + stringify(status.state()) + ")"')
    len('                : "");')
    
    Longest line is 78 characters..?



src/tests/master_tests.cpp
<https://reviews.apache.org/r/26701/#comment97854>

    Quite the long sentence ;)



src/tests/master_tests.cpp
<https://reviews.apache.org/r/26701/#comment97855>

    We should probably encode the release of resources in the name?
    
    ReleaseResourcesForTerminalTaskWithPendingUpdates


- Ben Mahler


On Oct. 17, 2014, 12:31 a.m., Vinod Kone wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26701/
> -----------------------------------------------------------
> 
> (Updated Oct. 17, 2014, 12:31 a.m.)
> 
> 
> Review request for mesos, Adam B, Ben Mahler, and Niklas Nielsen.
> 
> 
> Bugs: MESOS-1799 and MESOS-1817
>     https://issues.apache.org/jira/browse/MESOS-1799
>     https://issues.apache.org/jira/browse/MESOS-1817
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> Master now maintains the latest and unacknowledged states of the task.
> 
> 
> Diffs
> -----
> 
>   src/master/master.hpp 14f1d0fd240c9cd0718d0238e1fbb9c733190205 
>   src/master/master.cpp 0a5c9a374062a241c90ea238725fbb8dd2408ef4 
>   src/tests/master_tests.cpp d9dc40c6f5aaa66e1f7a0e1b7e4d9cdc586ca0fd 
> 
> Diff: https://reviews.apache.org/r/26701/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> Ran the new test 1000 times.
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>


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