<HTML><BODY><br><br><br><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;">
        Пятница,  6 июля 2018, 20:00 +03:00 от Konstantin Osipov <kostja@tarantool.org>:<br><br><div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_15308964360000000339_BODY">* Konstantin Belyavskiy <<a href="mailto:k.belyavskiy@tarantool.org">k.belyavskiy@tarantool.org</a>> [18/07/05 22:56]:<br>
                                 > Garbage collector do not delete xlog unless replica do not notify<br>
> master with newer vclock. This can lead to running out of disk<br>
> space error and this is not right behaviour since it will stop the<br>
> master.<br>
> Fix it by forcing gc to clean xlogs for replica with highest lag.<br>
> Add an error injection and a test.<br><br>
Please rebase this patch to the latest 1.10<br><br>
Please use relay_stop() as a callback to unregister the consumer.</div></div></div></div></blockquote>Rebase to 1.10 - ok.<br><br>Using relay_stop() makes sense only with replica_on_relay_stop(), since<br>relay_stop() itself actually do nothing with consumers.<br>Regarding replica_on_relay_stop(), replica should be in "orphan" mode<br>to avoid assertion in replica_delete(). And also there is a problem with<br>monitoring, since replica will leave replication cluster and thus silent the error.<br><br>On other hand, in case of implementation based on removing consumer,<br>replica, if being active again, will get an LSN gap and we will see an error.<br><br>1. Please give feedback on this section.<br>2. If not using relay_stop(), which branch use as a base 1.9 or 1.10?<br><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_15308964360000000339_BODY"><br>
> +void<br>
> +gc_xdir_clean_notify()<br>
> +{<br>
> +  /*<br>
> +   * Compare the current time with the time of the last run.<br>
> +   * This is needed in case of multiple failures to prevent<br>
> +   * from deleting all replicas.<br><br><br>
> +   */<br>
> +  static double prev_time = 0.;<br>
> +  double cur_time = ev_monotonic_time();<br>
> +  if (cur_time - prev_time < 1.)<br>
> +          return;<br><br>
This throttles gc, which is good. But we would still get a lot of messages<br>
from WAL thread. Maybe we should move the throttling to the WAL<br>
side? This would spare us from creating the message as well.<br>
Ideally we should use a single statically allocated message from<br>
the WAL for this purpose (but still throttle it as well).<br><br>
Plus, eventually you're going to reach a state when kicking off<br>
replicas doesn't help with space. In this case you're going to<br>
have a lot of messages, and they are going to be all useless.<br>
This also suggests that throttling should be done on the WAL side.<br></div></div></div></div></blockquote>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_15308964360000000339_BODY"><br>
> +  prev_time = cur_time;<br>
> +  struct gc_consumer *leftmost =<br>
> +      gc_tree_first(&gc.consumers);<br>
> +  /*<br>
> +   * Exit if no consumers left or if this consumer is<br>
> +   * not associated with replica (backup for example).<br>
> +   */<br><br>
> +  if (leftmost == NULL || leftmost->replica == NULL)<br>
> +          return;<br><br><br>
> +  /*<br>
> +   * We have to maintain @checkpoint_count oldest snapshots,<br>
> +   * plus we can't remove snapshots that are still in use.<br>
> +   * So if leftmost replica has signature greater or equel<br>
> +   * then the oldest checkpoint that must be preserved,<br>
> +   * nothing to do.<br>
> +   */<br><br>
This comment is useful, but the search in checkpoint array is not.<br>
What about possible other types of consumers which are not<br>
dispensable with anyway, e.g. backups? What if they are holding a<br>
reference as well? <br><br>
Apparently this check is taking care of the problem:</div></div></div></div></blockquote><br>In previous review you have already mentioned this problem, searching<br>in checkpoint array helps in case then last stored checkpoint has exactly<br>the same value.<br>But in general, check below already prevents replicas from deletion.<br>Should I keep it (searching in checkpoint array)?<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_15308964360000000339_BODY"><br><br>
> +  if (leftmost == NULL || leftmost->replica == NULL)<br>
> +          return;<br><br>
Could you write a test with two <br>
"abandoned" replicas, each holding an xlog file?</div></div></div></div></blockquote>Which xlog, the same one or different for each replicas?<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_15308964360000000339_BODY"><br><br>
-- <br>
Konstantin Osipov, Moscow, Russia, <span class="js-phone-number">+7 903 626 22 32</span><br><a href="http://tarantool.io" target="_blank">http://tarantool.io</a> - <a href="http://www.twitter.com/kostja_osipov" target="_blank">www.twitter.com/kostja_osipov</a><br><br></div></div></div></div></blockquote>
<br>
<br>Best regards,<br>Konstantin Belyavskiy<br>k.belyavskiy@tarantool.org<br></BODY></HTML>