Re: Review Request 22754: First part of the deactivate slaves mechanism - HTTP deactivate endpoint

[Ben Mahler <be...@gmail.com>]

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


Looking good, two things I think are missing before we can commit:

(1) Authentication (see Master::Http::shutdown for an example), since this endpoint affects the cluster. You may want to copy for now or pull out the authentication logic from Master::Http::shutdown into a Master helper function 'bool authenticated(const Request& request)', up to you.

(2) Let's add a test! We can test the various cases (invalid requests, valid requests: make sure offers are rescinded, and no more offers go out).

I think (2) will have helped us notice that this patch has a bug! We're not rescinding the offers for these slaves.


src/master/http.cpp
<https://reviews.apache.org/r/22754/#comment86622>

    Weird, not your fault but it looks like the decoding responsibility should be inside process::http::query::parse, not getFormValue! Feel free to leave a TODO to clean this up.



src/master/http.cpp
<https://reviews.apache.org/r/22754/#comment86619>

    How about /master/slaves/deactivate since we can deactivate frameworks as well?
    
    Make sure to do this when routing the endpoint as well.



src/master/http.cpp
<https://reviews.apache.org/r/22754/#comment86618>

    No need for the trailing spaces.



src/master/http.cpp
<https://reviews.apache.org/r/22754/#comment86617>

    Can you move hostnames up to the previous line?



src/master/http.cpp
<https://reviews.apache.org/r/22754/#comment86621>

    How about s/hostnamesString/value/ since this is the value for the hostnames key?



src/master/http.cpp
<https://reviews.apache.org/r/22754/#comment86623>

    Should we return a BadRequest if there are no hostnames in the vector?



src/master/http.cpp
<https://reviews.apache.org/r/22754/#comment86620>

    to_string is c++11, which isn't required yet for Mesos so we cannot rely on it.
    
    You can use strinfigy from stout:
    
    "Deactivated " + stringify(hostnames.size()) + " hosts.\n"
    
    Note you'll want to end with a newline. Can we also pass in the set of hostnames from the request and take the size here?



src/master/master.hpp
<https://reviews.apache.org/r/22754/#comment86626>

    Sorry, this is my fault, I think we should say "hostnames" when referring to the names of the hosts, and "hosts" when we are referring to the hosts themselves.
    
    So:
    
    // ... that run on the provided hosts.
    deactivateHosts(hostnames);
    
    Does that makes sense?



src/master/master.hpp
<https://reviews.apache.org/r/22754/#comment86624>

    Let's make this /master/slaves/deactivate per comment above?



src/master/master.cpp
<https://reviews.apache.org/r/22754/#comment86625>

    Let's make this /master/slaves/deactivate per comment above?



src/master/master.cpp
<https://reviews.apache.org/r/22754/#comment86632>

    s/deactivateSlaveIds/slaveIds/



src/master/master.cpp
<https://reviews.apache.org/r/22754/#comment86631>

    How about 'valid' and 'invalid'?



src/master/master.cpp
<https://reviews.apache.org/r/22754/#comment86630>

    How about just adding a - operator to hashset (in a dependent review) so that we can do:
    
    hashset<string> invalid = hostnames - valid;



src/master/master.cpp
<https://reviews.apache.org/r/22754/#comment86635>

    I think we'll want to do something different here when we have a blacklist, right?
    
    What if there are hostnames that don't match currently registered slaves, but that operators want to deactivate? We would still place these into the blacklist, so a TODO here might be good?



src/master/master.cpp
<https://reviews.apache.org/r/22754/#comment86633>

    Please wrap comments at 70 characters :)



src/master/master.cpp
<https://reviews.apache.org/r/22754/#comment86634>

    We need to rescind the offers here too. Let's add a tests per my comment at the top of the review so that we can catch bugs like this. :D


- Ben Mahler


On July 28, 2014, 4:48 p.m., Alexandra Sava wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22754/
> -----------------------------------------------------------
> 
> (Updated July 28, 2014, 4:48 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> This is the first part of the 'deactivate slave' mechanism.
> 
> This part consists of creating an HTTP endpoint which receives HTTP posts. Each post contains a JSON object with a list of host names.
> The list is further passed to the master which checks which slaves run on those hosts and marks them as 'deactivated' in the allocator. The final result is that the master will no longer send resource offers belonging to the deactivates slaves.
> 
> The TODO for the second part is to make the list of deactivated slaves persistent, by storing it into the Registry.
> 
> 
> Diffs
> -----
> 
>   src/master/http.cpp f2ca6599eb165c4c1bc4580175fa439f797c832b 
>   src/master/master.hpp d8a4d9e04ecff60020b99ea6447055787d187797 
>   src/master/master.cpp 273a516a964f586183557e270d34b55c5a34036b 
> 
> Diff: https://reviews.apache.org/r/22754/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Alexandra Sava
> 
>