Re: Review Request 68089: Added LevelDB compaction after replicated log truncation.

[Benjamin Mahler <bm...@apache.org>]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68089/#review209663
-----------------------------------------------------------




include/mesos/log/log.hpp
Lines 202-209 (patched)
<https://reviews.apache.org/r/68089/#comment294200>

    Can we avoid the additional constructors in favor of an additional default parameter?
    
    Alternatively, if you're willing to clean this up, we could introduce a Log::Options struct?



include/mesos/log/log.hpp
Lines 206 (patched)
<https://reviews.apache.org/r/68089/#comment294202>

    The comment doesn't mention "auto" compaction, can you mention it? Also, is MESOS-184 the clear pointer to what this is and when we can remove it (i.e. once the leveldb issue gets fixed and we upgrade the bundled leveldb?)



src/log/leveldb.hpp
Lines 45 (patched)
<https://reviews.apache.org/r/68089/#comment294203>

    Can you mention that this is only needed to support the auto-compaction feature (along with a pointer to that feature).



src/log/leveldb.cpp
Line 464 (original), 471 (patched)
<https://reviews.apache.org/r/68089/#comment294204>

    extra newline



src/log/leveldb.cpp
Lines 479 (patched)
<https://reviews.apache.org/r/68089/#comment294207>

    Are there error cases to handle?
    
    From what I could tell, it looks like this is an asynchronous operation that schedules the background compaction? Is it useful to time it given this?
    
    The log message here seems misleading if it's indeed done in the background?



src/log/log.hpp
Lines 53-55 (original), 53-74 (patched)
<https://reviews.apache.org/r/68089/#comment294208>

    Ditto here for avoiding the extra constructors



src/log/replica.hpp
Line 60 (original), 60 (patched)
<https://reviews.apache.org/r/68089/#comment294209>

    Can you document this flag including a pointer to why it was added and when it can be removed?



src/tests/log_tests.cpp
Lines 132-133 (original), 132-134 (patched)
<https://reviews.apache.org/r/68089/#comment294211>

    Can you explain that we want to test all cases with both auto-compaction on and off to ensure it has no correctness impact? Is this the only spot where we should be doing this?



src/tests/log_tests.cpp
Line 258 (original), 256 (patched)
<https://reviews.apache.org/r/68089/#comment294210>

    double semi-colon?


- Benjamin Mahler


On July 27, 2018, 8:12 p.m., Ilya Pronin wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68089/
> -----------------------------------------------------------
> 
> (Updated July 27, 2018, 8:12 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Jie Yu.
> 
> 
> Bugs: MESOS-184
>     https://issues.apache.org/jira/browse/MESOS-184
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> LevelDB compaction algorithm doesn't seem to work well with out key
> usage pattern and background compaction doesn't get triggered:
> https://github.com/google/leveldb/issues/603. Because of that the
> replicated log storage grows over time until it consumes all disk space
> on the partition. As a workaround this change adds manual
> leveldb::DB::CompactRange() invocation. Compaction is triggered after
> every truncation action persistence that involved removal of stored
> keys. By default this workaround is disabled and has to be turned on
> with autoCompact parameter of the Log constructor.
> 
> 
> Diffs
> -----
> 
>   include/mesos/log/log.hpp ba355331d682a7f2bdc534b4eb4402e2347528d8 
>   src/log/leveldb.hpp 1d5842a13cfe99fa3ea2ac73eabb2a0b55e15239 
>   src/log/leveldb.cpp 72eeffb17e2de39594de284bc01063445f82ab2c 
>   src/log/log.hpp 5612a1a2c4f0d3bba3d946b365f34616180eb426 
>   src/log/log.cpp 6a7080ab5115c777bd5039acb0f830563866781f 
>   src/log/replica.hpp 082c81cd92e1d46f8014cd23953878f9e23a19d7 
>   src/log/replica.cpp 25c128029c9d54f92145e2294f8f59d11b27da2a 
>   src/tests/log_tests.cpp a8980e3676f51d14087f56338ff45de2927ea992 
> 
> 
> Diff: https://reviews.apache.org/r/68089/diff/1/
> 
> 
> Testing
> -------
> 
> Updated `LogStorageTest` cases to run with and without compaction enabled. Ran `make check`.
> 
> 
> Thanks,
> 
> Ilya Pronin
> 
>