----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/8762/#review17260 -----------------------------------------------------------
Ship it! Great tests, pushes me to write better tests for my components! (usage() on the isolation modules in untested from my monitoring changes =/) Also, I thought you were going to pass in the SlaveID through the environment? src/exec/exec.cpp <https://reviews.apache.org/r/8762/#comment36696> Can we maybe spell this out in the public executor interfaces? Considering how harsh this is, I like the harshness if we have this contract, as people will _definitely_ discover they are doing this. One would probably assume the driver will store messages for later delivery if sent pre-registration. That would be a nicer contract than doing this, but I see the added complexity =/ src/exec/exec.cpp <https://reviews.apache.org/r/8762/#comment36697> ditto src/exec/exec.cpp <https://reviews.apache.org/r/8762/#comment36693> s/those// s/haven't/have not src/exec/exec.cpp <https://reviews.apache.org/r/8762/#comment36699> Were you going to add slave id to these? While you're at it: https://issues.apache.org/jira/browse/MESOS-334 Or maybe let's not do too many things in this change.. src/slave/cgroups_isolation_module.cpp <https://reviews.apache.org/r/8762/#comment36702> Can you split off the killed case in terms of the failed message? I see unknown as more serious than killed (since killed can happen due to known races). src/slave/cgroups_isolation_module.cpp <https://reviews.apache.org/r/8762/#comment36703> No need for a comment on this in the .cpp file since it's in the header, right? src/slave/cgroups_isolation_module.cpp <https://reviews.apache.org/r/8762/#comment36704> would be awful nice to pass that option through directly ;) src/slave/flags.hpp <https://reviews.apache.org/r/8762/#comment36705> great src/slave/process_based_isolation_module.cpp <https://reviews.apache.org/r/8762/#comment36708> ditto on splitting the failure message src/slave/slave.cpp <https://reviews.apache.org/r/8762/#comment36713> given the context here, "terminating" seems more appropriate you would set terminating to true, and then you call terminate, makes more sense IMO src/slave/slave.cpp <https://reviews.apache.org/r/8762/#comment36714> empty()? src/slave/slave.cpp <https://reviews.apache.org/r/8762/#comment36716> Does this imply the StatusUpdateManager provides idempotent primitives in general? src/slave/slave.cpp <https://reviews.apache.org/r/8762/#comment36717> Kill "If we are here, ", I know the comment is for this spot in the code because the comment was written in this spot for a reason! ;) src/slave/slave.cpp <https://reviews.apache.org/r/8762/#comment36718> comma after exited src/slave/slave.cpp <https://reviews.apache.org/r/8762/#comment36719> Isn't the reaper only for the process isolation module? src/slave/slave.cpp <https://reviews.apache.org/r/8762/#comment36720> quotes on executor id but not framework id? src/slave/slave.cpp <https://reviews.apache.org/r/8762/#comment36722> s/recovering/recovery of/ src/slave/slave.cpp <https://reviews.apache.org/r/8762/#comment36723> ditto src/slave/slave.cpp <https://reviews.apache.org/r/8762/#comment36725> s/framework/the framework entry/ src/slave/slave.cpp <https://reviews.apache.org/r/8762/#comment36727> s/Create executor/Create the executor entry/ src/slave/slave.cpp <https://reviews.apache.org/r/8762/#comment36728> kill newline? src/slave/slave.cpp <https://reviews.apache.org/r/8762/#comment36724> ditto src/slave/slave.cpp <https://reviews.apache.org/r/8762/#comment36729> s/,// src/slave/slave.cpp <https://reviews.apache.org/r/8762/#comment36730> s/by/for/? src/slave/slave.cpp <https://reviews.apache.org/r/8762/#comment36732> else? src/slave/slave.cpp <https://reviews.apache.org/r/8762/#comment36733> s/Setup the monitoring./Begin monitoring the executor./ src/tests/slave_recovery_tests.cpp <https://reviews.apache.org/r/8762/#comment36734> Line wrap at 70 - Ben Mahler On Feb. 27, 2013, 7:59 p.m., Vinod Kone wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/8762/ > ----------------------------------------------------------- > > (Updated Feb. 27, 2013, 7:59 p.m.) > > > Review request for mesos, Benjamin Hindman and Ben Mahler. > > > Description > ------- > > Properly recovers and reconnects with executors. > > Also, recovers isolation module. > > This is pretty much the whole of recovery! (I'm going to send out another > tiny review for properly doing incompatible upgrades) > > > Diffs > ----- > > src/exec/exec.cpp 821a94fab1f5969183ecf9e28d7b6bc10920db24 > src/launcher/launcher.hpp ead58c0f60b6bff532f241a224b547b25e292175 > src/launcher/launcher.cpp f2d68c5fd1201b75c5a8f96599d7fb36516687fb > src/launcher/main.cpp e90df85ae433431defdd251490c74a482a58f743 > src/messages/messages.proto 815fcbbcb4a8643f50950a294cedf7281b2a187f > src/slave/cgroups_isolation_module.hpp > a04fc46b15d2741886f5847cadbdc9bed351c588 > src/slave/cgroups_isolation_module.cpp > a779de80d13c67e507d7d2ee788fcdaa5e71daca > src/slave/constants.hpp 08db75f3e5af0ae79298546386deddf00beb8de9 > src/slave/constants.cpp 159dd12ec1e6b5f33fd75fc3a66d8ac897fc40b7 > src/slave/flags.hpp 9cd436cae5772b906b3566eb1f344904d5894214 > src/slave/isolation_module.hpp d7cc02b8ea0cde591ee61059efd79fbacaa74769 > src/slave/lxc_isolation_module.hpp 6be99038fb547e43e74ef5469f2339bbe82017cf > src/slave/lxc_isolation_module.cpp 1ed3c87f884089e032bba2ed89be109abe6c1c13 > src/slave/process_based_isolation_module.hpp > 6a4e6aef9ab4b13cce9341f6f35c6b24e6f8fde7 > src/slave/process_based_isolation_module.cpp > ff98d105af675dfc66070feaa43b42c1aa438fd8 > src/slave/reaper.cpp c0ee4b4c07fd792bcb39455b666808b712eb32c2 > src/slave/slave.hpp 7648c33230c1900eda7529045c5df9ccab105d47 > src/slave/slave.cpp 8c2e1bfc363491c681177676f9dfe5f229276f7d > src/slave/state.cpp 2688567719c5b12f1d8a56fa6193c11d4077685b > src/slave/status_update_manager.cpp PRE-CREATION > src/tests/slave_recovery_tests.cpp PRE-CREATION > src/tests/utils.hpp 4600c2f6e0ceabd6a38f8bb762da314b795c23a0 > > Diff: https://reviews.apache.org/r/8762/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Vinod Kone > >
