> 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.
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". > 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? 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. > On Aug. 23, 2012, 6:18 a.m., Benjamin Hindman wrote: > > src/slave/slave.cpp, line 1580 > > <https://reviews.apache.org/r/6704/diff/2/?file=143659#file143659line1580> > > > > It is a bit weird to see a Future<Try<>>, but I'm guessing you plan to > > eliminate that once you make the functions return a Future directly via the > > async stuff? > > Vinod Kone wrote: > i'm not sure what you mean by 'eliminate'? after the async stuff is > added, this would look like > > Future<Try<double> > usage = async(os::usage); > I was thinking you would 'async()' the statvfs function, chain with a then (or a libprocess process), and thus just return the future. The benefit of doing it the way you have done it is that we don't have to introduce Future into stout (which I don't want to). So, I think the next abstraction that we need is something which takes a Future<Try<T>> and returns a Future<T> pulling out the Try value or the Try error! Yeah! - Benjamin ----------------------------------------------------------- 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 > >
