> On Aug. 23, 2012, 6:18 a.m., Benjamin Hindman wrote: > > src/slave/slave.cpp, line 1565 > > <https://reviews.apache.org/r/6704/diff/2/?file=143659#file143659line1565> > > > > s/checkDiskUsage/getDiskUsage/ > > Vinod Kone wrote: > a void get() seems counter-intuitive. check() for me implies we are going > to do something about it? > > Benjamin Hindman wrote: > I agree, 'check' for me does imply we are going to do something about it, > but that's not what this function does (that's what a later function does, > what you called 'garbageCollect'). To me this is really a two-phase process: > (1) get the current usage and (2) check if the current usage requires extra > garbage collecting.
To me, the fact that garbage collection happens asynchronously inside check() is an implementation detail. Ideally the garbageCollect() function should be private (I just pulled it up to public to make it visible for testing). In other words, garbageCollect() is something we don't expect users of Slave should be calling directly. I'm also in favor of s/garbageCollect()/_garbageCollect()/ s/check()/garbageCollect()/ thoughts? > On Aug. 23, 2012, 6:18 a.m., Benjamin Hindman wrote: > > src/slave/slave.cpp, line 1578 > > <https://reviews.apache.org/r/6704/diff/2/?file=143659#file143659line1578> > > > > s/garbageCollect/checkDiskUsage/ > > Vinod Kone wrote: > this function is actually scheduling things for garbage collection. i'm > open to a better alternative than check() here. > > Benjamin Hindman wrote: > The problem I have with "garbageCollect" is that this function is > intrinsically tied to garbage collecting _after_ checking disk usage. Reading > 'garbageCollect' to me does not imply: "check resource usage and then > possibly garbage collect", it implies: "garbage collect resources". See my comment above. Also, how about thinking garbageCollect() as doing garbage collection 'based on' resource usage. If it can take Try<double> instead of Future<Try <double>>, this would have been less confusing? - Vinod ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/6704/#review10671 ----------------------------------------------------------- On Aug. 23, 2012, 7:43 p.m., Vinod Kone wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/6704/ > ----------------------------------------------------------- > > (Updated Aug. 23, 2012, 7:43 p.m.) > > > Review request for mesos, Benjamin Hindman, John Sirois, and Ben Mahler. > > > Description > ------- > > Added the ability in the slave to GC executor directories based on the > current disk usage. > > Currently, it uses a very simple algorithm to decide which directories to > delete. > > > This addresses bug MESOS-254. > https://issues.apache.org/jira/browse/MESOS-254 > > > Diffs > ----- > > src/slave/constants.hpp ab83972 > src/slave/flags.hpp 0c7917f > src/slave/gc.hpp 6704742 > src/slave/gc.cpp 9c01024 > src/slave/slave.hpp 10c537b > src/slave/slave.cpp 4efd41e > src/tests/gc_tests.cpp 2f0bdde > third_party/libprocess/include/stout/os.hpp b1eceb3 > third_party/libprocess/include/stout/time.hpp 46d7077 > > Diff: https://reviews.apache.org/r/6704/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Vinod Kone > >
