mesos-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Vinod Kone" <vinodk...@gmail.com>
Subject Re: Review Request: Slave GC based on disk usage
Date Mon, 27 Aug 2012 20:56:50 GMT


> 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?
> 
> Benjamin Hindman wrote:
>     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.

To me, the fact that garbage collection happens asynchronously inside check() is an implementation
detail. Ideally the garbageCollect() function should be private (I just pulled it up to public
to make it visible for testing). In other words, garbageCollect() is something we don't expect
users of Slave should be calling directly.

I'm also in favor of
s/garbageCollect()/_garbageCollect()/
s/check()/garbageCollect()/

thoughts?


> 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.
> 
> Benjamin Hindman wrote:
>     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".

See my comment above. 

Also, how about thinking garbageCollect() as doing garbage collection 'based on' resource
usage. If it can take Try<double> instead of Future<Try <double>>, this
would have been less confusing?


- Vinod


-----------------------------------------------------------
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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message