<HTML><BODY><br>Thank you for the review.<br><br><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;">
Четверг, 28 июня 2018, 18:25 +03:00 от Konstantin Osipov <kostja@tarantool.org>:<br><br><div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_15301995410000000419_BODY">* Konstantin Belyavskiy <<a href="mailto:k.belyavskiy@tarantool.org">k.belyavskiy@tarantool.org</a>> [18/06/28 18:13]:<br>
> <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>
> + static int prev_time = 0;<br>
> + int cur_time = time(NULL);<br>
> + if (cur_time - prev_time < 1)<br><br>
Please use fiber time.</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_15301995410000000419_BODY"><br><br>
> + /**<br>
> + * Mark consumer with least recent vclock as "dead" and<br>
> + * invoke garbage collection. If nothing to delete find<br>
> + * next alive consumer etc. Originally created for<br>
> + * cases with running out of disk space because of<br>
> + * disconnected replica.<br>
> + */<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>
You should delete the consumer *only* if it's going to help with<br>
garbage. Running out of space should not lead to deletion of all<br>
replicas as consumers if they are not lagging behind.</div></div></div></div></blockquote>Here I delete only the consumer with the least recent vclock<br>(or consumers if they share the same value) since gc_tree use<br>vclock signature for comparison (see gc_consumer_cmp).<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_15301995410000000419_BODY"><br>
> + if (leftmost == NULL || leftmost->replica == NULL)<br>
> + return;<br>
> + int64_t signature = leftmost->signature;<br>
> + while (true) {<br>
> + gc_consumer_unregister(leftmost);<br>
> + leftmost = gc_tree_first(&gc.consumers);<br>
> + if (leftmost == NULL || leftmost->replica == NULL ||<br>
> + leftmost->signature > signature) {<br>
> + gc_run();<br>
> + return;<br>
> + }<br>
> + }<br>
> +}<br>
> +<br><br>
<cut><br><br>
> const char *wal_mode_STRS[] = { "none", "write", "fsync", NULL };<br>
> @@ -64,6 +65,8 @@ struct wal_thread {<br>
> * priority pipe and DOES NOT support yield.<br>
> */<br>
> struct cpipe tx_prio_pipe;<br>
> + /** Return pipe from 'wal' to tx' */<br>
> + struct cpipe tx_pipe;<br><br>
Why do you need a new pipe?</div></div></div></div></blockquote>Since operations with file (like deleting of old xlogs) requires yield and<br>tx_prio does not support it.<br>'tx' is a fiber pool, but 'tx_prio' is an endpoint and supports only callbacks<br>without 'yield'.<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_15301995410000000419_BODY"><br>
> <br>
> +static void<br>
> +gc_status_update(struct cmsg *msg)<br>
> +{<br>
> + gc_xdir_clean_notify();<br>
> + delete(msg);<br><br>
delete()?!</div></div></div></div></blockquote>Sorry, my fault.<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_15301995410000000419_BODY"><br><br>
> {<br>
> @@ -655,6 +665,15 @@ done:<br>
> /* Until we can pass the error to tx, log it and clear. */<br>
> error_log(error);<br>
> diag_clear(diag_get());<br>
> + if (errno == ENOSPC) {<br>
> + struct cmsg *msg =<br>
> + (struct cmsg*)calloc(1, sizeof(struct cmsg));<br><br>
Please check calloc() return value and do nothing <br>
in case it's NULL.</div></div></div></div></blockquote>Done.<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_15301995410000000419_BODY"><br>
> + static const struct cmsg_hop route[] = {<br>
> + {gc_status_update, NULL}<br>
> + };<br>
> + cmsg_init(msg, route);<br>
> + cpipe_push(&wal_thread.tx_pipe, msg);<br>
> + }<br>
> <br>
> index 895d938d5..11f1b7fdc 100644<br>
> +...<br>
> +-- add a little timeout so gc could finish job<br>
> +require('fiber').sleep(0.01)<br><br>
Please never sleep in a test case unconditionally. It doesn't work<br>
reliably, especially in parallel runs.<br><br>
The test case should test that replicas which are severely behind <br>
are abandoned, and replicas which are running well are not.</div></div></div></div></blockquote>Ok, will replace it with new test case.<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_15301995410000000419_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>