<HTML><BODY><br><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;">
Воскресенье, 21 октября 2018, 23:41 +03:00 от Alexander Turenko <alexander.turenko@tarantool.org>:<br><br><div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_15401544820000000538_BODY">Hi!<br><br>
I don't have objections in general. Some minor comments are below.<br><br>
Please, answer with fixes, don't just send the whole patch.<br><br>
WBR, Alexander Turenko.<br><br>
> Instead of using timeout we need just pause `relay_send`. Can't rely<br>
> on timeout because of various system load in parallel mode. Add new<br>
> errinj which checks boolean in loop and until it is not `True` do not<br>
> pass the method `relay_send` to the next statement.<br>
> <br>
> To check the read-only mode, need to make a modification of tuple. It<br>
> is enough to call `replace` method. Instead of `delete` and then<br>
> useless verification that we have not delete space by using `get`<br>
> method.<br>
> <br><br>
delete space -> delete tuple?</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_15401544820000000538_BODY"><br><br>
> +-- In the next two cases we try to replace a tuple while replica<br>
> +-- is catching up with the master (local delete, remote delete)<br><br>
delete -> replace<br></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_15401544820000000538_BODY"><br>
> +-- case<br><br>
Nit: period at the end.</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_15401544820000000538_BODY"><br><br>
> --- check sync<br>
> -errinj.set("ERRINJ_RELAY_TIMEOUT", 0)<br>
> +-- Resume replicaton.<br><br>
replicaton -> replication<br></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_15401544820000000538_BODY"><br>
> diff --git a/test/replication/gc.test.lua b/test/replication/gc.test.lua<br>
> index 5100378b3..22921289d 100644<br>
> --- a/test/replication/gc.test.lua<br>
> +++ b/test/replication/gc.test.lua<br>
> @@ -12,6 +12,7 @@ default_checkpoint_count = box.cfg.checkpoint_count<br>
> box.cfg{checkpoint_count = 1}<br>
> <br>
> function wait_gc(n) while #box.info.gc().checkpoints > n do fiber.sleep(0.01) end end<br>
> +function wait_xlog(n, timeout) timeout = timeout or 1.0 return test_run:wait_cond(function() return #fio.glob('./master/*.xlog') == n end, timeout) end<br>
><br><br>
Use 'set delimiter' and write it in several lines. Also, below I<br>
proposed to support 'n' being a table to allow count of files being one<br>
of several values. You can use auxiliary function like the following and<br>
type(n) == 'table' check.<br><br>
function value_in(val, arr)<br>
for _, elem in ipairs(arr) do<br>
if val == elem then<br>
return true<br>
end<br>
end<br>
return false<br>
end<br></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_15401544820000000538_BODY"><br>
> @@ -31,7 +32,7 @@ for i = 1, 100 do s:auto_increment{} end<br>
> <br>
> -- Make sure replica join will take long enough for us to<br>
> -- invoke garbage collection.<br>
> -box.error.injection.set("ERRINJ_RELAY_TIMEOUT", 0.05)<br>
> +box.error.injection.set("ERRINJ_RELAY_SEND_DELAY", true)<br>
> <br>
> -- While the replica is receiving the initial data set,<br>
> -- make a snapshot and invoke garbage collection, then<br>
> @@ -41,7 +42,7 @@ test_run:cmd("setopt delimiter ';'")<br>
> fiber.create(function()<br>
> fiber.sleep(0.1)<br>
> box.snapshot()<br>
> - box.error.injection.set("ERRINJ_RELAY_TIMEOUT", 0)<br>
> + box.error.injection.set("ERRINJ_RELAY_SEND_DELAY", false)<br>
> end)<br>
> test_run:cmd("setopt delimiter ''");<br>
> <br><br>
The entire comment:<br><br>
> -- While the replica is receiving the initial data set,<br>
> -- make a snapshot and invoke garbage collection, then<br>
> -- remove the timeout injection so that we don't have to<br>
> -- wait too long for the replica to start.<br><br>
Proposed: then remove delay to allow replica to start.<br></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_15401544820000000538_BODY"><br>
> --- Remove the timeout injection so that the replica catches<br>
> +-- Resume replicaton so that the replica catches<br><br>
replicaton -> replication<br></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_15401544820000000538_BODY"><br>
> @@ -146,17 +147,16 @@ box.snapshot()<br>
> _ = s:auto_increment{}<br>
> box.snapshot()<br>
> #box.info.gc().checkpoints == 1 or box.info.gc()<br>
> -xlog_count = #fio.glob('./master/*.xlog')<br>
> -- the replica may have managed to download all data<br>
> -- from xlog #1 before it was stopped, in which case<br>
> -- it's OK to collect xlog #1<br>
> -xlog_count == 3 or xlog_count == 2 or fio.listdir('./master')<br>
> +wait_xlog(3, 0.1) or wait_xlog(2, 0.1) or fio.listdir('./master')<br><br>
You are set timeout to 1.0 for other cases, but 0.2 here. So, 0.2 is<br>
enough? It is better to allow the function to accept a table like {2, 3}<br>
as files count. Use 'set delimiter' and update the function.<br></div></div></div></div></blockquote>
fixed<br>
<br>-- <br>Sergei Voronezhskii<br></BODY></HTML>