----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/47973/#review136592 -----------------------------------------------------------
src/slave/gc.cpp (line 135) <https://reviews.apache.org/r/47973/#comment201682> Add a comment here so it's clear why we are doing this. ``` // Run rmdir with 'continueOnError = true'. It's possible for // tasks and isolators to lay down files that are not deletable by // GC. In the face of such errors GC needs to free up disk space // wherever it can because it's already re-offered to frameworks. ``` src/tests/gc_tests.cpp (lines 70 - 71) <https://reviews.apache.org/r/47973/#comment201687> Remember to remove these now that we don't use subprocesses anymore. src/tests/gc_tests.cpp (line 856) <https://reviews.apache.org/r/47973/#comment201683> Let's fix this test to use a busy mount point for the same reason as in /r/45959/ src/tests/gc_tests.cpp (line 857) <https://reviews.apache.org/r/47973/#comment201684> 1. Two space indentation. 2. `}` on a new line. src/tests/gc_tests.cpp (lines 862 - 870) <https://reviews.apache.org/r/47973/#comment201686> Let's do this ``` if (mountPoint.isSome()) { fs::unmount(mountPoint.get(), MNT_FORCE | MNT_DETACH); } ``` src/tests/gc_tests.cpp (line 880) <https://reviews.apache.org/r/47973/#comment201688> s/ImmutableFile/BusyMountPoint/ src/tests/gc_tests.cpp (line 899) <https://reviews.apache.org/r/47973/#comment201743> Even though this fits in one line, it's more customary to always put continuations on new lines. src/tests/gc_tests.cpp (line 908) <https://reviews.apache.org/r/47973/#comment201744> Even though `offers` being ready should guarantee that `frameworkId` is ready, let's still `AWAIT_READY(frameworkId)` explictly above `AWAIT_READY(offers);`. src/tests/gc_tests.cpp (lines 913 - 914) <https://reviews.apache.org/r/47973/#comment201753> Comment on the significance of the file names: ``` // The busy mount point goes before the regular file in GC's // directory traversal due to their names. This makes sure that // an error occurs before all deletable files are GCed. ``` src/tests/gc_tests.cpp (lines 919 - 921) <https://reviews.apache.org/r/47973/#comment201751> For multi-line command strings it's cleaner to do: ``` "touch " + regularFile + "; " "mkdir " + mountPoint + "; " "mount --bind " + mountPoint + " " + mountPoint, ``` src/tests/gc_tests.cpp (lines 922 - 923) <https://reviews.apache.org/r/47973/#comment201745> Put each argument, i.e., None(), "test-task123", "test-task123" on a new line. src/tests/gc_tests.cpp (lines 948 - 956) <https://reviews.apache.org/r/47973/#comment201752> Why do we still need `containerPaths`? We can get the sandbox path directly: ``` const string sandbox = slave::paths::getExecutorLatestRunPath( flags.work_dir, slaveId, frameworkId.get(), executor_id); ASSERT_TRUE(os::exists(sandbox)); ``` right? src/tests/gc_tests.cpp (line 975) <https://reviews.apache.org/r/47973/#comment201755> It should work without `.value` due to implicit conversion. src/tests/gc_tests.cpp (line 996) <https://reviews.apache.org/r/47973/#comment201754> Kill the blank line. - Jiang Yan Xu On May 27, 2016, 5:02 p.m., Megha Sharma wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/47973/ > ----------------------------------------------------------- > > (Updated May 27, 2016, 5:02 p.m.) > > > Review request for mesos and Jiang Yan Xu. > > > Bugs: MESOS-5196 > https://issues.apache.org/jira/browse/MESOS-5196 > > > Repository: mesos > > > Description > ------- > > https://issues.apache.org/jira/browse/MESOS-5196 > Updates made to mesos gc to prevent early exit in the event > of error. > > > Diffs > ----- > > src/slave/gc.cpp eedb8ca713d7f5195055bb6417f03ab70731af97 > src/tests/gc_tests.cpp 4cb7c2f612984f7f5a9378a7f972f2438bbf28c5 > > Diff: https://reviews.apache.org/r/47973/diff/ > > > Testing > ------- > > make check. > Tested with option --gtest_also_run_disabled_tests. > > > Thanks, > > Megha Sharma > >
