> 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! > > Benjamin Hindman wrote: > 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)?
aha. i see. fixed. but i still think, timer.creator() better captures the fact that it has not been initialized yet (and it fits on one line :)))? > 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. > > Benjamin Hindman wrote: > 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! I think both, mocking gc and os, are great solutions. I will add a TODO. I don't think mocking just gc doesn't help us with this test, untill os is mocked out. > 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? > > Benjamin Hindman wrote: > I think we just move this to the 'fs' namespace (object! ;) in the near > future. sure. but, i dont think fs::du() would be any clear. - Vinod ----------------------------------------------------------- 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 > >
