> 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()) { > > Vinod Kone wrote: > 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!
Just to be clear, if some time has elapsed, shouldn't remaining be 0 (note that the semantics of remaining are _not_ return negative time). And if no time has elapsed, shouldn't remaining be 0 (since timeout == Clock::now)? > 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! > > Vinod Kone wrote: > i'm guessing you have a reason to be inconclusive about this? fixed for > now. Here's my reasoning: in many languages an assert might get removed from a non-debug build, so putting stuff in assert statements was a big no no. In our case, CHECK never gets removed, but it feels enough like an assert that I don't like the habit of sticking stuff in there. > 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? > > Vinod Kone wrote: > 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. I think two things would need to get mocked in this case. First, you'd want to mock GarbageCollector (and inject it via the constructor) so that we can see when we get prune calls. Second, you'd want to mock os::usage so that it returns a usage value which wold cause the slave to call prune. The former seems trivial, the latter, not so much. Here's one suggestion (we should seriously think about this so that we can write proper tests): create a class OS which includes all of our functions (either as virtual functions or via fast delegates), create a global instance of that called 'os', and replace all calls to 'os::xxx' with 'os.xxx'. Then, any time you want to mock one of those functions, just replace the global 'os' variable with a subclass of OS that mocks what's necessary (i.e., overrides a virtual function or replaces a delegate mapping). Thoughts on this? We shouldn't include this in the review, but re-evaluate it in the near future! > 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? > > Vinod Kone wrote: > 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? I think we just move this to the 'fs' namespace (object! ;) in the near future. - Benjamin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/6704/#review11304 ----------------------------------------------------------- On Sept. 11, 2012, 11:06 p.m., Vinod Kone wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/6704/ > ----------------------------------------------------------- > > (Updated Sept. 11, 2012, 11:06 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 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 > >
