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