> On Sept. 11, 2012, 5:10 a.m., Benjamin Hindman wrote: > > src/slave/flags.hpp, line 97 > > <https://reviews.apache.org/r/6704/diff/7/?file=151993#file151993line97> > > > > As I mentioned before, I really would prefer that we mention that this > > number is really just an upper bound and changes based on disk usage, > > something like: > > > > Maximum amount of time to wait before cleaning up\n > > executor directories (e.g., 3days, 2 weeks, etc).\n > > Note that this delay may be shorter depending on\n > > available disk usage
I swear I fixed this before, but somehow got some how got lost in my rebase quagmire. fixed > On Sept. 11, 2012, 5:10 a.m., Benjamin Hindman wrote: > > src/slave/gc.cpp, line 49 > > <https://reviews.apache.org/r/6704/diff/7/?file=151995#file151995line49> > > > > Previously I've embedded a struct like this directly into the class > > (just above the map that holds these). See src/zookeeper/group.cpp. done > On Sept. 11, 2012, 5:10 a.m., Benjamin Hindman wrote: > > src/slave/gc.cpp, line 93 > > <https://reviews.apache.org/r/6704/diff/7/?file=151995#file151995line93> > > > > The timer.creator() seems weird to me. What about capturing exactly the > > semantics you care about doing something like: > > > > if (timer.timeout().remaining() == Seconds(0) || removalTime < > > timer.timeout()) { i don't think that would work? how would we guarantee that timer.timeout().remaining() is 0? afaict, 'Timer' initializes 'timeout' to Clock::now() and by the time .remaining() is called, some time might have got elapsed! > On Sept. 11, 2012, 5:10 a.m., Benjamin Hindman wrote: > > src/slave/gc.cpp, line 154 > > <https://reviews.apache.org/r/6704/diff/7/?file=151995#file151995line154> > > > > What is the reason to dispatch here? Because I don't want remove() to modify 'paths', while prune() is iterating over it. > On Sept. 11, 2012, 5:10 a.m., Benjamin Hindman wrote: > > src/slave/slave.cpp, line 1543 > > <https://reviews.apache.org/r/6704/diff/7/?file=151997#file151997line1543> > > > > Please don't crash the slave if for whatever reason a file gets added > > to the directory that is not a number. done > On Sept. 11, 2012, 5:10 a.m., Benjamin Hindman wrote: > > src/slave/slave.cpp, line 1547 > > <https://reviews.apache.org/r/6704/diff/7/?file=151997#file151997line1547> > > > > We haven't come to a conclusion on the style for putting things inside > > of a CHECK (except for in tests). For now, please pull it out (like the > > deleted code) until we decide what we want to do. Thanks! i'm guessing you have a reason to be inconclusive about this? fixed for now. > On Sept. 11, 2012, 5:10 a.m., Benjamin Hindman wrote: > > third_party/libprocess/include/stout/os.hpp, line 586 > > <https://reviews.apache.org/r/6704/diff/7/?file=152001#file152001line586> > > > > Any reason not to call this du? I debated quite a bit about naming this guy. I didn't want to use 'du' because I didn't want give an impression that this is like the 'du' unix command. Because, its more like 'df', in the sense that this only cares about file systems not directories. On that note, I will change its argument to fs instead of path, to make this explicit. And I didn't want to call it 'df' because we are actually returning 'used' ratio, not the 'free' ratio. I was torn between using 'usage' or 'capacity' (what 'df' spits out as the header), but decided go with the former because its more explicit. Thoughts? > On Sept. 11, 2012, 5:10 a.m., Benjamin Hindman wrote: > > src/tests/gc_tests.cpp, line 448 > > <https://reviews.apache.org/r/6704/diff/7/?file=151998#file151998line448> > > > > Once _checkDiskUsage becomes a lambda, you won't be able to test it > > this way ... any thoughts on how to write a test then? yea, its tricky! short of mocking the slave, i'm not sure what other options we have. 'gc' itself is a private variable, so we cannot call its prune() method directly. may be we can pull up 'gc', to make it visible for testing? but, that doesnt' answer your concern about proliferating private variables up. is there way we could 'catch' a dispatch, mid flight, and change its contents? that seems very hard and an over-kill. > On Sept. 11, 2012, 5:10 a.m., Benjamin Hindman wrote: > > src/slave/slave.hpp, line 128 > > <https://reviews.apache.org/r/6704/diff/7/?file=151996#file151996line128> > > > > I do not think we should proliferate this kind of thing. Please see the > > comment in the test itself as well. i agree, see my comment below in test. - Vinod ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/6704/#review11304 ----------------------------------------------------------- On Sept. 10, 2012, 6:39 a.m., Vinod Kone wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/6704/ > ----------------------------------------------------------- > > (Updated Sept. 10, 2012, 6:39 a.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 380a2b9 > src/slave/flags.hpp e99ee8b > src/slave/gc.hpp 3760d09 > src/slave/gc.cpp 5212a41 > src/slave/slave.hpp b7ab2ab > src/slave/slave.cpp 3e2a8d5 > src/tests/gc_tests.cpp bb7416c > third_party/libprocess/include/process/timeout.hpp ce0dea7 > third_party/libprocess/include/stout/duration.hpp e303d2c > third_party/libprocess/include/stout/os.hpp df0f7ff > > Diff: https://reviews.apache.org/r/6704/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Vinod Kone > >
