<HTML><BODY><br><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;">
Четверг, 28 июня 2018, 16:00 +03:00 от Vladimir Davydov <vdavydov.dev@gmail.com>:<br><br><div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_15301908290000000477_BODY">On Thu, Jun 28, 2018 at 03:09:08PM +0300, Serge Petrenko wrote:<br>
> Garbage collection doesn't distinguish consumers which need checkpoint<br>
> files, such as backup, and the ones, who only need WALS, such as<br>
> replicas. A disconnected replica will 'hold' all checkpoint files, created<br>
> after it got unsynchronised, even though it doesn't need them, which may<br>
> lead to disk space shortage. To fix this, we store consumer's type, and<br>
> treat consumers differently during garbage collection: now only the old<br>
> WALS are stored for replicas, and old checkpoints are stored for backup,<br>
> if any. Also changed the tests to check updated garbage collection correctly.<br>
> <br>
> Closes #3444<br>
> ---<br>
> <a href="https://github.com/tarantool/tarantool/tree/sergepetrenko/gh-3444-remove-old-shapshots-for-replicas" target="_blank">https://github.com/tarantool/tarantool/tree/sergepetrenko/gh-3444-remove-old-shapshots-for-replicas</a><br>
> <a href="https://github.com/tarantool/tarantool/issues/3444" target="_blank">https://github.com/tarantool/tarantool/issues/3444</a><br><br>
After fixing your code according to review comments, you should amend<br>
your old commit, not make a new one (take a look at `git commit --amend`<br>
and `git revert -i`), then send the amended patch to the mailing list.<br>
Sometimes, when the change set is fairly small (which isn't the case<br>
this time), you might want to send an incremental diff in reply to the<br>
email with review remarks, but you should amend your branch anyway.<br>
Please do. Also, see a few minor comments below.</div></div></div></div></blockquote>Sorry, all done.<br><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_15301908290000000477_BODY"><br>
> <br>
> Changes in v2:<br>
> - prefixed variable names with prefix 'checkpoint_'<br>
> instead of 'snap_', so that there is no<br>
> confusion with memtx snapshots<br>
> - same with changing variable name<br>
> xlog_only to wal_only<br>
> - rewrote gc_run so that there is only a single<br>
> loop over checkpoints, and also one excess old<br>
> WAL is removed (it was unneeded, but kept due<br>
> to a mistake). Now wal_collect_garbage or<br>
> engine_collect_garbage are called only if they<br>
> have work to do.<br>
> - fix tests to correctly check the amount of xlogs<br>
> kept by garbage collection<br>
> <br>
> src/box/gc.c | 90 +++++++++++++++++++-------------------------<br>
> src/box/gc.h | 10 ++---<br>
> test/replication/gc.result | 26 ++++++++++++-<br>
> test/replication/gc.test.lua | 14 ++++++-<br>
> 4 files changed, 80 insertions(+), 60 deletions(-)<br>
> <br>
> diff --git a/src/box/gc.c b/src/box/gc.c<br>
> index 288cc7236..06d2fbd35 100644<br>
> --- a/src/box/gc.c<br>
> +++ b/src/box/gc.c<br>
> @@ -161,12 +161,12 @@ gc_free(void)<br>
> latch_destroy(&gc.latch);<br>
> }<br>
> <br>
> -/** Find the consumer that uses the oldest snapshot */<br>
> +/** Find the consumer that uses the oldest checkpoint */<br><br>
Comment-style: a dot at the end of the sentence is missing.</div></div></div></div></blockquote>Fixed.<br><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_15301908290000000477_BODY"><br>
> struct gc_consumer *<br>
> -gc_first_snap(gc_tree_t *consumers)<br>
> +gc_tree_first_checkpoint(gc_tree_t *consumers)<br>
> {<br>
> struct gc_consumer *consumer = gc_tree_first(consumers);<br>
> - while (consumer != NULL && consumer->xlog_only)<br>
> + while (consumer != NULL && consumer->wal_only)<br>
> consumer = gc_tree_next(consumers, consumer);<br>
> return consumer;<br>
> }<br>
> @@ -179,15 +179,16 @@ gc_run(void)<br>
> <br>
> /* Look up the consumer that uses the oldest WAL */<br>
> struct gc_consumer *leftmost = gc_tree_first(&gc.consumers);<br>
> - /* Look up the consumer that uses the oldest snapshot. */<br>
> - struct gc_consumer *leftmost_snap = gc_first_snap(&gc.consumers);<br>
> + /* Look up the consumer that uses the oldest checkpoint. */<br>
> + struct gc_consumer *leftmost_checkpoint =<br>
> + gc_tree_first_checkpoint(&gc.consumers);<br><br>
Coding-style: when continuing an expression to the next line, indent it<br>
with tabs, please:<br><br>
struct gc_consumer *leftmost_checkpoint =<br>
gc_tree_first_checkpoint(&gc.consumers).</div></div></div></div></blockquote>Fixed.<br><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_15301908290000000477_BODY"><br>
> <br>
> /*<br>
> * Find the oldest checkpoint that must be preserved.<br>
> - * We have to maintain @checkpoint_count oldest snapshots,<br>
> - * plus we can't remove snapshots that are still in use.<br>
> + * We have to maintain @checkpoint_count oldest checkpoints,<br>
> + * plus we can't remove checkpoints that are still in use.<br>
> */<br>
> - int64_t gc_xlog_signature = -1;<br>
> + int64_t gc_checkpoint_signature = -1;<br>
> <br>
> struct checkpoint_iterator checkpoints;<br>
> checkpoint_iterator_init(&checkpoints);<br>
> @@ -196,37 +197,20 @@ gc_run(void)<br>
> while ((vclock = checkpoint_iterator_prev(&checkpoints)) != NULL) {<br>
> if (--checkpoint_count > 0)<br>
> continue;<br>
> - if (leftmost != NULL &&<br>
> - leftmost->signature < vclock_sum(vclock))<br>
> + if (leftmost_checkpoint != NULL &&<br>
> + leftmost_checkpoint->signature < vclock_sum(vclock))<br>
> continue;<br>
> - gc_xlog_signature = vclock_sum(vclock);<br>
> + gc_checkpoint_signature = vclock_sum(vclock);<br>
> break;<br>
> }<br>
> <br>
> - int64_t gc_snap_signature = -1;<br>
> - checkpoint_count = gc.checkpoint_count;<br>
> + int64_t gc_wal_signature = MIN(gc_checkpoint_signature, leftmost != NULL ?<br>
> + leftmost->signature : INT64_MAX);<br><br>
Ditto.</div></div></div></div></blockquote>Fixed.<br><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_15301908290000000477_BODY"><br>
> <br>
> - checkpoint_iterator_init(&checkpoints);<br>
> -<br>
> - while ((vclock = checkpoint_iterator_prev(&checkpoints)) != NULL) {<br>
> - if (--checkpoint_count > 0)<br>
> - continue;<br>
> - if (leftmost_snap != NULL &&<br>
> - leftmost_snap->signature < vclock_sum(vclock))<br>
> - continue;<br>
> - gc_snap_signature = vclock_sum(vclock);<br>
> - break;<br>
> - }<br>
> -<br>
> - if (gc_snap_signature <= gc.snap_signature &&<br>
> - gc_xlog_signature <= gc.xlog_signature)<br>
> + if (gc_checkpoint_signature <= gc.checkpoint_signature &&<br>
> + gc_wal_signature <= gc.wal_signature)<br>
> return; /* nothing to do */<br>
> <br>
> - if (gc_snap_signature > gc.snap_signature)<br>
> - gc.snap_signature = gc_snap_signature;<br>
> - if (gc_xlog_signature > gc.xlog_signature)<br>
> - gc.xlog_signature = gc_xlog_signature;<br>
> -<br>
> /*<br>
> * Engine callbacks may sleep, because they use coio for<br>
> * removing files. Make sure we won't try to remove the<br>
> @@ -234,6 +218,7 @@ gc_run(void)<br>
> * executions.<br>
> */<br>
> latch_lock(&gc.latch);<br>
> +<br>
> /*<br>
> * Run garbage collection.<br>
> *<br><br>
Please try to avoid stray hunks, like this one, when preparing a patch.<br>
They complicate review.</div></div></div></div></blockquote>Wym?<br><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_15301908290000000477_BODY"><br>
> @@ -241,8 +226,17 @@ gc_run(void)<br>
> * collection for memtx snapshots first and abort if it<br>
> * fails - see comment to memtx_engine_collect_garbage().<br>
> */<br>
> - if (engine_collect_garbage(gc_snap_signature) == 0)<br>
> - wal_collect_garbage(gc_xlog_signature);<br>
> + int rc = 0;<br>
> +<br>
> + if (gc_checkpoint_signature > gc.checkpoint_signature) {<br>
> + gc.checkpoint_signature = gc_checkpoint_signature;<br>
> + rc = engine_collect_garbage(gc_checkpoint_signature);<br>
> + }<br>
> + if (gc_wal_signature > gc.wal_signature) {<br>
> + gc.wal_signature = gc_wal_signature;<br>
> + if (rc == 0)<br>
> + wal_collect_garbage(gc_wal_signature);<br>
> + }<br><br>
I guess, in case engine_collect_garbage() fails and we don't call<br>
wal_collect_garbage(), we shouldn't advance gc.wal_signature:<br><br>
if (rc == 0 && gc_wal_signature > gc.wal_signature) {<br>
gc.wal_signature = gc_wal_signature;<br>
wal_collect_garbage(gc_wal_signature);<br>
}<br></div></div></div></div></blockquote>
That's how it was done previously. First update gc.signature, and only then<br>try to run encgine_collect_garbage() and wal_collect_garbage(), so I decided<br>to leave it as is.<br><br><p>---<br> src/box/gc.c | 10 +++++-----<br> 1 file changed, 5 insertions(+), 5 deletions(-)</p><p>diff --git a/src/box/gc.c b/src/box/gc.c<br>index 06d2fbd35..4d8f321ff 100644<br>--- a/src/box/gc.c<br>+++ b/src/box/gc.c<br>@@ -161,7 +161,7 @@ gc_free(void)<br> latch_destroy(&gc.latch);<br> }<br> <br>-/** Find the consumer that uses the oldest checkpoint */<br>+/** Find the consumer that uses the oldest checkpoint. */<br> struct gc_consumer *<br> gc_tree_first_checkpoint(gc_tree_t *consumers)<br> {<br>@@ -181,7 +181,7 @@ gc_run(void)<br> struct gc_consumer *leftmost = gc_tree_first(&gc.consumers);<br> /* Look up the consumer that uses the oldest checkpoint. */<br> struct gc_consumer *leftmost_checkpoint =<br>- gc_tree_first_checkpoint(&gc.consumers);<br>+ gc_tree_first_checkpoint(&gc.consumers);<br> <br> /*<br> * Find the oldest checkpoint that must be preserved.<br>@@ -198,17 +198,17 @@ gc_run(void)<br> if (--checkpoint_count > 0)<br> continue;<br> if (leftmost_checkpoint != NULL &&<br>- leftmost_checkpoint->signature < vclock_sum(vclock))<br>+ leftmost_checkpoint->signature < vclock_sum(vclock))<br> continue;<br> gc_checkpoint_signature = vclock_sum(vclock);<br> break;<br> }<br> <br> int64_t gc_wal_signature = MIN(gc_checkpoint_signature, leftmost != NULL ?<br>- leftmost->signature : INT64_MAX);<br>+ leftmost->signature : INT64_MAX);<br> <br> if (gc_checkpoint_signature <= gc.checkpoint_signature &&<br>- gc_wal_signature <= gc.wal_signature)<br>+ gc_wal_signature <= gc.wal_signature)<br> return; /* nothing to do */<br> <br> /*<br>-- <br>2.15.2 (Apple Git-101.1)</p><br>
<br>-- <br>Best regards,<br>Sergey Petrenko<br></BODY></HTML>