----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32505/#review78415 -----------------------------------------------------------
Ship it! Modulo comments. I noticed you added SlaveID on Shutdown, can you add it to Kill as well? include/mesos/scheduler/scheduler.proto <https://reviews.apache.org/r/32505/#comment127126> Do you want to put this below KILL? include/mesos/scheduler/scheduler.proto <https://reviews.apache.org/r/32505/#comment127142> "Shuts down a custom executor" "When the executor" include/mesos/scheduler/scheduler.proto <https://reviews.apache.org/r/32505/#comment127144> "If the" include/mesos/scheduler/scheduler.proto <https://reviews.apache.org/r/32505/#comment127146> "transition" include/mesos/scheduler/scheduler.proto <https://reviews.apache.org/r/32505/#comment127151> How about a newline for readability? ``` // transitions its active tasks to TASK_LOST. // // TODO(vinod): ... ``` include/mesos/scheduler/scheduler.proto <https://reviews.apache.org/r/32505/#comment127192> Should you add a required SlaveID to Kill as well since that's the same API as Shutdown? include/mesos/scheduler/scheduler.proto <https://reviews.apache.org/r/32505/#comment127161> Below Kill? src/master/master.hpp <https://reviews.apache.org/r/32505/#comment127163> Above reconcile if you put it below Kill in the API? src/master/master.cpp <https://reviews.apache.org/r/32505/#comment127164> Ditto here. src/master/master.cpp <https://reviews.apache.org/r/32505/#comment127168> Whoops, this should already be saying either "exited with status X" or "terminated with signal X". src/master/master.cpp <https://reviews.apache.org/r/32505/#comment127170> Shouldn't this TODO be in the slave..? src/master/master.cpp <https://reviews.apache.org/r/32505/#comment127171> Ditto about ordering w.r.t. other calls. src/master/master.cpp <https://reviews.apache.org/r/32505/#comment127172> Did you forget the 'return' here? src/master/master.cpp <https://reviews.apache.org/r/32505/#comment127174> Looks like you forgot to put the `return` statement here? :) It would be great if you could re-use `drop` here, either by doing the `Slave*` lookup in `receive`, much like we do the framework lookup and drop if not found, or by adding a `drop` overload. But the former seems cleaner. src/slave/slave.cpp <https://reviews.apache.org/r/32505/#comment127181> "shutdownFramework"..? Do you need this comment now that you've split out a separate `_shutdownExecutor` function? src/slave/slave.cpp <https://reviews.apache.org/r/32505/#comment127187> The top two LOG(WARNING)s are wrapped on the next line, but the next two are wrapped at the first line, mind just wrapping them all at the first line? src/slave/slave.cpp <https://reviews.apache.org/r/32505/#comment127186> Mind putting the space in front of "yet" instead of at the end of "not"? Makes it a bit clearer that the spaces are correct IMHO because its quick to scan down a straight line with the eyes as opposed to the jagged end of the lines (and we do this above in this function :)). I remember I discussed wrapping || and && conditionals at the front of the lines (for the same reason), but that ship has sailed.. src/tests/scheduler_tests.cpp <https://reviews.apache.org/r/32505/#comment127188> Ditto from my other review, don't you need this above the Mesos construction? src/tests/scheduler_tests.cpp <https://reviews.apache.org/r/32505/#comment127189> newline here? src/tests/scheduler_tests.cpp <https://reviews.apache.org/r/32505/#comment127190> newline here? src/tests/scheduler_tests.cpp <https://reviews.apache.org/r/32505/#comment127191> newline here? - Ben Mahler On March 31, 2015, 12:09 a.m., Vinod Kone wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/32505/ > ----------------------------------------------------------- > > (Updated March 31, 2015, 12:09 a.m.) > > > Review request for mesos, Benjamin Hindman and Ben Mahler. > > > Bugs: MESOS-1127 and MESOS-330 > https://issues.apache.org/jira/browse/MESOS-1127 > https://issues.apache.org/jira/browse/MESOS-330 > > > Repository: mesos > > > Description > ------- > > This is a new call added to explicitly shutdown an executor. > > > Diffs > ----- > > include/mesos/scheduler/scheduler.proto > 783a63ad1cc0edd86605d638046fb959cb6e97e8 > src/master/master.hpp 05be911734b8d70c9fa5f3b4a275e8b610621fc5 > src/master/master.cpp 618db68ee4163b06e479cf3413eda4b63c9c5a4b > src/messages/messages.proto 97c45c01dfcea38b1ae555c036d61e10c152c2c8 > src/scheduler/scheduler.cpp 584b042e32865fdf875bf41ebcfb7f9c327d882a > src/slave/slave.hpp 19e6b44bc344c0ca509674803f401cbb4e1f47ae > src/slave/slave.cpp 0f70ebafb0f5b16c560decc66e22a43a52732d09 > src/tests/scheduler_tests.cpp 4a89a7a88b50bb8c254f5076661ce07ac9fc7657 > > Diff: https://reviews.apache.org/r/32505/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Vinod Kone > >
