> On April 1, 2015, 12:20 a.m., Ben Mahler wrote: > > Modulo comments. > > > > I noticed you added SlaveID on Shutdown, can you add it to Kill as well?
Yup. Will send a patch for it as well. > On April 1, 2015, 12:20 a.m., Ben Mahler wrote: > > include/mesos/scheduler/scheduler.proto, line 127 > > <https://reviews.apache.org/r/32505/diff/2/?file=910158#file910158line127> > > > > Do you want to put this below KILL? I reordered them all in the very last review of this chain. > On April 1, 2015, 12:20 a.m., Ben Mahler wrote: > > include/mesos/scheduler/scheduler.proto, line 174 > > <https://reviews.apache.org/r/32505/diff/2/?file=910158#file910158line174> > > > > "Shuts down a custom executor" > > "When the executor" done. > On April 1, 2015, 12:20 a.m., Ben Mahler wrote: > > include/mesos/scheduler/scheduler.proto, line 176 > > <https://reviews.apache.org/r/32505/diff/2/?file=910158#file910158line176> > > > > "If the" done > On April 1, 2015, 12:20 a.m., Ben Mahler wrote: > > include/mesos/scheduler/scheduler.proto, line 180 > > <https://reviews.apache.org/r/32505/diff/2/?file=910158#file910158line180> > > > > "transition" done > On April 1, 2015, 12:20 a.m., Ben Mahler wrote: > > include/mesos/scheduler/scheduler.proto, line 188 > > <https://reviews.apache.org/r/32505/diff/2/?file=910158#file910158line188> > > > > Should you add a required SlaveID to Kill as well since that's the same > > API as Shutdown? will do it in a separate review. > On April 1, 2015, 12:20 a.m., Ben Mahler wrote: > > include/mesos/scheduler/scheduler.proto, line 235 > > <https://reviews.apache.org/r/32505/diff/2/?file=910158#file910158line235> > > > > Below Kill? Reordered in the last review of this chain. > On April 1, 2015, 12:20 a.m., Ben Mahler wrote: > > src/master/master.hpp, lines 463-465 > > <https://reviews.apache.org/r/32505/diff/2/?file=910159#file910159line463> > > > > Above reconcile if you put it below Kill in the API? see above > On April 1, 2015, 12:20 a.m., Ben Mahler wrote: > > src/master/master.cpp, lines 1608-1613 > > <https://reviews.apache.org/r/32505/diff/2/?file=910160#file910160line1608> > > > > Ditto here. will do in the final review where i reorder everything. > On April 1, 2015, 12:20 a.m., Ben Mahler wrote: > > src/master/master.cpp, line 3454 > > <https://reviews.apache.org/r/32505/diff/2/?file=910160#file910160line3454> > > > > Shouldn't this TODO be in the slave..? added there too. > On April 1, 2015, 12:20 a.m., Ben Mahler wrote: > > src/master/master.cpp, lines 3475-3493 > > <https://reviews.apache.org/r/32505/diff/2/?file=910160#file910160line3475> > > > > Ditto about ordering w.r.t. other calls. will do once in the final review. > On April 1, 2015, 12:20 a.m., Ben Mahler wrote: > > src/master/master.cpp, lines 3481-3485 > > <https://reviews.apache.org/r/32505/diff/2/?file=910160#file910160line3481> > > > > 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. Added return. Good catch! Not all calls have slave id in them unlike framework id. So looking up Slave* in receive doesn't seem correct? > On April 1, 2015, 12:20 a.m., Ben Mahler wrote: > > src/slave/slave.cpp, lines 3433-3437 > > <https://reviews.apache.org/r/32505/diff/2/?file=910164#file910164line3433> > > > > 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? done > On April 1, 2015, 12:20 a.m., Ben Mahler wrote: > > src/slave/slave.cpp, lines 3451-3452 > > <https://reviews.apache.org/r/32505/diff/2/?file=910164#file910164line3451> > > > > 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.. done > On April 1, 2015, 12:20 a.m., Ben Mahler wrote: > > src/tests/scheduler_tests.cpp, lines 346-349 > > <https://reviews.apache.org/r/32505/diff/2/?file=910165#file910165line346> > > > > Ditto from my other review, don't you need this above the Mesos > > construction? no. per my comment in the previous review. > On April 1, 2015, 12:20 a.m., Ben Mahler wrote: > > src/tests/scheduler_tests.cpp, line 384 > > <https://reviews.apache.org/r/32505/diff/2/?file=910165#file910165line384> > > > > newline here? done > On April 1, 2015, 12:20 a.m., Ben Mahler wrote: > > src/tests/scheduler_tests.cpp, line 386 > > <https://reviews.apache.org/r/32505/diff/2/?file=910165#file910165line386> > > > > newline here? done > On April 1, 2015, 12:20 a.m., Ben Mahler wrote: > > src/tests/scheduler_tests.cpp, line 407 > > <https://reviews.apache.org/r/32505/diff/2/?file=910165#file910165line407> > > > > newline here? done - Vinod ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32505/#review78415 ----------------------------------------------------------- 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 > >
