<HTML><BODY>I didn't submit it because it's reveal problems with different replication timeout on two instances. For test purposes two instance run with different replication timeouts and I have to add several workarounds. But on production setup we can have same problems. It was functional before and this change breaks it.<br>So decided with Roman to develop new schema.<br><br><br><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;">
Пятница, 2 февраля 2018, 23:32 +03:00 от Konstantin Osipov <kostja@tarantool.org>:<br>
<br>
<div id="">
<div class="js-helper js-readmsg-msg">
<style type="text/css"></style>
<div>
<base target="_self" href="https://e.mail.ru/">
<div id="style_15176035560000000525_BODY">Hi,<br>
<br>
I don't see this patch submitted for review in patches@. Is it<br>
ready for review?<br>
<br>
* Konstantin Belyavskiy <<a href="mailto:k.belyavskiy@tarantool.org">k.belyavskiy@tarantool.org</a>> [18/02/02 16:57]:<br>
<br>
> commit e470923d8142641da7ef7b898d1ead00a439ab0b<br>
> Author: Konstantin Belyavskiy <<a href="mailto:k.belyavskiy@tarantool.org">k.belyavskiy@tarantool.org</a>><br>
> AuthorDate: Thu Feb 1 19:17:58 2018 +0300<br>
> <br>
> Break connection on timeout<br>
> <br>
> In replication schema if one of the instances was powered off, it doesn't detected<br>
> by others and the connection hangs. Alive machines show 'follow' state.<br>
> Add timeout to solve this issue. It's safe since applier and relay both send<br>
> messages every replication_timeout so we can assume that if we read nothing we<br>
> have problem with connection.<br>
> Use replication_disconnect_timeout which is replication_timeout * 4 as for now.<br>
<br>
Please align around 65 characters.<br>
<br>
> <br>
> Closes #3025<br>
> ---<br>
> src/box/applier.cc | 12 +++++++++++-<br>
> src/errinj.h | 1 +<br>
> test/replication/errinj.result | 13 +++++++++++++<br>
> test/replication/errinj.test.lua | 7 +++++++<br>
> 4 files changed, 32 insertions(+), 1 deletion(-)<br>
> <br>
> diff --git a/src/box/applier.cc b/src/box/applier.cc<br>
> index f0073ba..94add65 100644<br>
> --- a/src/box/applier.cc<br>
> +++ b/src/box/applier.cc<br>
> @@ -45,6 +45,7 @@<br>
> #include "version.h"<br>
> #include "trigger.h"<br>
> #include "xrow_io.h"<br>
> +#include "errinj.h"<br>
> #include "error.h"<br>
> #include "session.h"<br>
> <br>
> @@ -418,7 +419,16 @@ applier_subscribe(struct applier *applier)<br>
> applier_set_state(applier, APPLIER_FOLLOW);<br>
> }<br>
> <br>
> - coio_read_xrow(coio, ibuf, &row);<br>
> + if (applier->version_id < version_id(1, 7, 7))<br>
> + coio_read_xrow(coio, ibuf, &row);<br>
> + else {<br>
> + double timeout = replication_disconnect_timeout();<br>
> + struct errinj *inj = errinj(ERRINJ_APPLIER_TIMEOUT,<br>
> + ERRINJ_DOUBLE);<br>
> + if (inj != NULL && inj->dparam > 0)<br>
> + timeout = inj->dparam;<br>
> + coio_read_xrow_timeout_xc(coio, ibuf, &row, timeout);<br>
> + }<br>
> <br>
<br>
Why do you need to check for version? Because 1.7.7 and lower has<br>
no heartbeats, is this correct? This needs a comment.<br>
<br>
> if (iproto_type_is_error(row.type))<br>
> xrow_decode_error_xc(&row); /* error */<br>
> diff --git a/src/errinj.h b/src/errinj.h<br>
> index 512d234..a5296cd 100644<br>
> --- a/src/errinj.h<br>
> +++ b/src/errinj.h<br>
> @@ -105,6 +105,7 @@ struct errinj {<br>
> _(ERRINJ_BUILD_SECONDARY, ERRINJ_INT, {.iparam = -1}) \<br>
> _(ERRINJ_VY_POINT_ITER_WAIT, ERRINJ_BOOL, {.bparam = false}) \<br>
> _(ERRINJ_RELAY_EXIT_DELAY, ERRINJ_DOUBLE, {.dparam = 0}) \<br>
> + _(ERRINJ_APPLIER_TIMEOUT, ERRINJ_DOUBLE, {.dparam = 0}) \<br>
> <br>
> ENUM0(errinj_id, ERRINJ_LIST);<br>
> extern struct errinj errinjs[];<br>
> diff --git a/test/replication/errinj.result b/test/replication/errinj.result<br>
> index d1f1dbe..3b0b345 100644<br>
> --- a/test/replication/errinj.result<br>
> +++ b/test/replication/errinj.result<br>
> @@ -426,6 +426,19 @@ test_run:cmd("switch replica_ack")<br>
> ---<br>
> - true<br>
> ...<br>
> +-- applier connection timeout is proportional to replication timeout, so since it's very low for this<br>
> +-- test case, we should manually set it to new value to allow connection. It should be higher than<br>
> +-- default's replication_timeout (0.1)<br>
> +box.error.injection.set("ERRINJ_APPLIER_TIMEOUT", 1.)<br>
<br>
This may prove to be not enough under load. Better to set it to a<br>
very large value, say, 24*60*60<br>
0.0 <br>
> +---<br>
> +- ok<br>
> +...<br>
> +fiber = require('fiber')<br>
> +---<br>
> +...<br>
> +fiber.sleep(0.5)<br>
> +---<br>
<br>
I'm not ready to pay +0.4 second for this test, so perhaps we need to <br>
set the timeout to an even lower value, say 0.05 seconds.<br>
Please ensure also that you check for status reaching the desired<br>
condition in a sleep-check loop, and not once - otherwise your<br>
test is guaranteed to sporadically fail.<br>
<br>
> +...<br>
> box.info.replication[1].upstream.status<br>
> ---<br>
> - follow<br>
> diff --git a/test/replication/errinj.test.lua b/test/replication/errinj.test.lua<br>
> index ba83481..803b0a4 100644<br>
> --- a/test/replication/errinj.test.lua<br>
> +++ b/test/replication/errinj.test.lua<br>
> @@ -175,8 +175,15 @@ for i = 0, 9999 do box.space.test:replace({i, 4, 5, 'test'}) end<br>
> test_run:cmd("create server replica_ack with rpl_master=default, script='replication/replica_ack.lua'")<br>
> test_run:cmd("start server replica_ack")<br>
> test_run:cmd("switch replica_ack")<br>
> +-- applier connection timeout is proportional to replication timeout, so since it's very low for this<br>
> +-- test case, we should manually set it to new value to allow connection. It should be higher than<br>
> +-- default's replication_timeout (0.1)<br>
<br>
Please align comments around 66 characters, never more than 80.<br>
<br>
> +box.error.injection.set("ERRINJ_APPLIER_TIMEOUT", 1.)<br>
> +fiber = require('fiber')<br>
> +fiber.sleep(0.5)<br>
> box.info.replication[1].upstream.status<br>
> <br>
> +<br>
> test_run:cmd("stop server default")<br>
> test_run:cmd("deploy server default")<br>
> test_run:cmd("start server default")<br>
<br>
-- <br>
Konstantin Osipov, Moscow, Russia, <span class="js-phone-number">+7 903 626 22 32</span><br>
<a href="http://tarantool.org" target="_blank">http://tarantool.org</a> - <a href="http://www.twitter.com/kostja_osipov" target="_blank">www.twitter.com/kostja_osipov</a><br>
</div>
<base target="_self" href="https://e.mail.ru/">
</div>
</div>
</div>
</blockquote>
<br>
<br>С уважением,<br>Konstantin Belyavskiy<br>k.belyavskiy@tarantool.org<br></BODY></HTML>