----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70335/#review214247 -----------------------------------------------------------
src/slave/slave.hpp Lines 889 (patched) <https://reviews.apache.org/r/70335/#comment300461> Why do we need this hashmap? I see that the code inserts/removes entries, but nothing ever gets anything from it. src/slave/slave.cpp Lines 4420-4421 (patched) <https://reviews.apache.org/r/70335/#comment300451> `message.framework_info()` might not be set if the operation is triggered by an operator and not a framework, so these variables should probably be options. src/slave/slave.cpp Lines 4433 (patched) <https://reviews.apache.org/r/70335/#comment300452> Nit: I'd name this `unscheduleGC`. src/slave/slave.cpp Lines 4439-4443 (patched) <https://reviews.apache.org/r/70335/#comment300458> Should we also unschedule the garbage collection of the framework work directory? It gets scheduled for GC here: https://github.com/apache/mesos/blob/1acb38c27306326a53f866b5386b5e28a6dc9314/src/slave/slave.cpp#L6834-L6838 src/slave/slave.cpp Lines 4452-4456 (patched) <https://reviews.apache.org/r/70335/#comment300456> Nit: fits in one line. src/slave/slave.cpp Lines 4459 (patched) <https://reviews.apache.org/r/70335/#comment300457> I noticed that `Slave::run()` only checkpoints the framework if `frameworkInfo.checkpoint()` returns `true`. Should we maybe not checkpoint operations/frameworkInfos of frameworks that set `checkpoint` to `false`? src/slave/slave.cpp Lines 4479 (patched) <https://reviews.apache.org/r/70335/#comment300459> Nit: I'd say: "Failed to unschedule framework directories scheduled for gc" I would also include the path that couldn't be gc'd. - Gastón Kleiman On March 28, 2019, 4:27 p.m., Greg Mann wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70335/ > ----------------------------------------------------------- > > (Updated March 28, 2019, 4:27 p.m.) > > > Review request for mesos, Benjamin Bannier, Benno Evers, Chun-Hung Hsiao, > Gastón Kleiman, Joseph Wu, and Meng Zhu. > > > Bugs: MESOS-8582 > https://issues.apache.org/jira/browse/MESOS-8582 > > > Repository: mesos > > > Description > ------- > > This patch updates the master to send a framework's full > `FrameworkInfo` to the agent in the `ApplyOperationMessage`. > The agent is updated to checkpoint frameworks when applying > operations, and to unschedule GC on the meta directory when > a new framework is created. > > The test `TerminalOrphanOperationAfterMasterFailover` is > removed since this patch eliminates the case of orphan > operations relevant to that test. > > > Diffs > ----- > > src/master/master.cpp acc67d3763ddee9027e6cf375f1d495ff5805026 > src/messages/messages.proto e30ad34cc9212b05f85ba5e1d4fcfc9e49ae92c0 > src/slave/slave.hpp 2bffdc4b1e282d3c6dae2dcf23584a8a4550bf94 > src/slave/slave.cpp 5373cee5d30c2403497939eeba2ee5405117237e > src/tests/persistent_volume_tests.cpp > 7e929a5a3a92e16a5dec10206f37caebc20d66a8 > src/tests/reservation_tests.cpp cd84cd24d3587fafc01ae1861f22c47262f2d7e9 > src/tests/storage_local_resource_provider_tests.cpp > 55c9389453754227de31e0a76d32ba663cc8ca7c > > > Diff: https://reviews.apache.org/r/70335/diff/1/ > > > Testing > ------- > > `make check` > > > Thanks, > > Greg Mann > >
