----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/14663/#review27421 -----------------------------------------------------------
Ship it! src/slave/slave.cpp <https://reviews.apache.org/r/14663/#comment53284> I see, we could get around this if we always updated the mtime of the previous slave directory before creating a slave directory. I realize this is non-trivial, just leaving it here as a note for posterity. src/slave/slave.cpp <https://reviews.apache.org/r/14663/#comment53285> We should likely check if it exists rather than crashing if we cannot update the utime: if (os::exists(path)) { // There is a race here if it gets deleted underneath us! CHECK_SOME(os::utime(path)); // Update the modification time. garbageCollect(path); } Or simply omit the CHECK_SOME: os::utime(path); garbageCollect(path); Or only garbageCollect if we can update the time: if (os::utime(path).isSome()) { garbageCollect(path); } It seems like there are cases where some of these files could be non-existent and we should not crash in these cases. src/slave/slave.cpp <https://reviews.apache.org/r/14663/#comment53283> This opens up a race condition between checking whether the file exists and looking at the mtime. If the file is removed in between the slave will crash. We can avoid this by just doing: { Try<long> mtime = os::mtime(path); if (mtime.isError()) { return Future<Nothing>::failed(mtime.error()); } // GC based on the modification time. Duration delay = flags.gc_delay - (Clock::now().duration() - Seconds(mtime.get())); return gc.schedule(delay, path); } Since mtime will return an error for missing files already, why not just use that error? - Ben Mahler On Oct. 23, 2013, 7:45 p.m., Vinod Kone wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/14663/ > ----------------------------------------------------------- > > (Updated Oct. 23, 2013, 7:45 p.m.) > > > Review request for mesos, Benjamin Hindman and Ben Mahler. > > > Bugs: MESOS-742 > https://issues.apache.org/jira/browse/MESOS-742 > > > Repository: mesos-git > > > Description > ------- > > Old recovered directories are now gc'ed based on modification time. > > > Diffs > ----- > > src/slave/slave.hpp 22fb74b71a0f52d9d67b92ecc286fa8d350e41a4 > src/slave/slave.cpp debb2f4ce05fbfec450197e68bc8a0c78f1d0adf > > Diff: https://reviews.apache.org/r/14663/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Vinod Kone > >
