> On Aug. 23, 2012, 6:18 a.m., Benjamin Hindman wrote: > > src/slave/flags.hpp, line 96 > > <https://reviews.apache.org/r/6704/diff/2/?file=143655#file143655line96> > > > > I think we should mention that this is actually just an upper bound, > > but as usage goes up, this number changes (i.e., explain the "age" > > function).
done > On Aug. 23, 2012, 6:18 a.m., Benjamin Hindman wrote: > > src/slave/flags.hpp, line 106 > > <https://reviews.apache.org/r/6704/diff/2/?file=143655#file143655line106> > > > > No need for this newline. done > On Aug. 23, 2012, 6:18 a.m., Benjamin Hindman wrote: > > src/slave/flags.hpp, line 126 > > <https://reviews.apache.org/r/6704/diff/2/?file=143655#file143655line126> > > > > It will be a nice addition to be able to make all of these doubles > > either seconds, hours, or more likely, "duration" objects. Technical debt. > > :/ Added a TODO for duration abstraction. I will get to this in another review. Also, filed https://issues.apache.org/jira/browse/MESOS-260 > On Aug. 23, 2012, 6:18 a.m., Benjamin Hindman wrote: > > src/slave/gc.cpp, line 41 > > <https://reviews.apache.org/r/6704/diff/2/?file=143657#file143657line41> > > > > Include vector in this file. done > On Aug. 23, 2012, 6:18 a.m., Benjamin Hindman wrote: > > src/slave/gc.cpp, line 85 > > <https://reviews.apache.org/r/6704/diff/2/?file=143657#file143657line85> > > > > Why the 'creator' bit? to set the timer for the very first time. its a round-about way of checking the timer is not set. i'm open to a better/clear way to capture this? > On Aug. 23, 2012, 6:18 a.m., Benjamin Hindman wrote: > > src/slave/gc.cpp, line 101 > > <https://reviews.apache.org/r/6704/diff/2/?file=143657#file143657line101> > > > > No need for ending period here! ;) Ask me later why. done. but why? to be consistent with other cases where LOG statements end with variables? > On Aug. 23, 2012, 6:18 a.m., Benjamin Hindman wrote: > > src/slave/gc.cpp, line 104 > > <https://reviews.apache.org/r/6704/diff/2/?file=143657#file143657line104> > > > > { done > On Aug. 23, 2012, 6:18 a.m., Benjamin Hindman wrote: > > src/slave/slave.hpp, line 166 > > <https://reviews.apache.org/r/6704/diff/2/?file=143658#file143658line166> > > > > Is usage a percent? nope, just a fraction > 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/ a void get() seems counter-intuitive. check() for me implies we are going to do something about it? > 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/ this function is actually scheduling things for garbage collection. i'm open to a better alternative than check() here. > On Aug. 23, 2012, 6:18 a.m., Benjamin Hindman wrote: > > src/slave/slave.cpp, line 1569 > > <https://reviews.apache.org/r/6704/diff/2/?file=143659#file143659line1569> > > > > Why not s/capacity/usage/? i was going for what df prints out. but, i guess 'usage' is fine too. fixed > 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? 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); > On Aug. 23, 2012, 6:18 a.m., Benjamin Hindman wrote: > > src/slave/gc.cpp, line 59 > > <https://reviews.apache.org/r/6704/diff/2/?file=143657#file143657line59> > > > > It would be more clear to factor the pair<string, Promise<bool>*> out > > into a struct. i couldn't find any structs in state.hpp. did u mean class? anyway, i used a simple struct > On Aug. 23, 2012, 6:18 a.m., Benjamin Hindman wrote: > > src/tests/gc_tests.cpp, line 162 > > <https://reviews.apache.org/r/6704/diff/2/?file=143660#file143660line162> > > > > YES YES YES YES YES! I so much prefer this! We made createTask be so > > damn simple to reason about. Hurray! > > > > Note, eventually one could imagine making this a nice abstraction that > > all tests could use to creates tasks. glad, u like it :) > On Aug. 23, 2012, 6:18 a.m., Benjamin Hindman wrote: > > src/slave/gc.cpp, line 95 > > <https://reviews.apache.org/r/6704/diff/2/?file=143657#file143657line95> > > > > I'll suggest 'reset' instead of 'next' for your deliberation. done - Vinod ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/6704/#review10671 ----------------------------------------------------------- On Aug. 21, 2012, 11:20 p.m., Vinod Kone wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/6704/ > ----------------------------------------------------------- > > (Updated Aug. 21, 2012, 11:20 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 > > Diff: https://reviews.apache.org/r/6704/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Vinod Kone > >
