<HTML><BODY><br>branch: gh-3025-break-connection-timeout-v2<br><br><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;">
Четверг, 8 февраля 2018, 20:05 +03:00 от Vladimir Davydov <vdavydov.dev@gmail.com>:<br><br><div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_15181095050000000895_BODY">On Thu, Feb 08, 2018 at 07:42:46PM +0300, Konstantin Belyavskiy wrote:<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>
Personally, I don't think we need the new option for now, hardcocded<br>
replication_timeout * 4 (see replication_disconnect_timeout() helper)<br>
will do just fine IMHO. If anybody asks for it (nobody has yet), we can<br>
introduce it anytime later. To simulate the server freeze in the test,<br>
you can use the existing ERRINJ_RELAY_REPORT_INTERVAL.<br><br>
Anyway, if you do think the option is absolutely necessary, let's do one<br>
step at a time - first fix the actual bug, only then add the new option<br>
in a separate patch.<br><br>
> <br>
> Closes #3025<br>
> ---<br>
> src/box/applier.cc | 7 ++++++-<br>
> src/box/box.cc | 23 ++++++++++++++++++++-<br>
> src/box/box.h | 1 +<br>
> src/box/lua/cfg.cc | 13 ++++++++++++<br>
> src/box/lua/load_cfg.lua | 4 ++++<br>
> src/box/relay.cc | 2 +-<br>
> src/box/replication.cc | 1 +<br>
> src/box/replication.h | 7 ++-----<br>
> test/app-tap/init_script.result | 43 ++++++++++++++++++++--------------------<br>
> test/box/admin.result | 2 ++<br>
> test/box/cfg.result | 4 ++++<br>
> test/replication/errinj.result | 13 ++++++++++++<br>
> test/replication/errinj.test.lua | 7 +++++++<br>
> 13 files changed, 98 insertions(+), 29 deletions(-)<br>
> <br>
> diff --git a/src/box/applier.cc b/src/box/applier.cc<br>
> index f0073bada..bc1f8fc5e 100644<br>
> --- a/src/box/applier.cc<br>
> +++ b/src/box/applier.cc<br>
> @@ -418,7 +418,12 @@ 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><br>
A comment why you need to check a version here is a must.</div></div></div></div></blockquote>Agree, 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_15181095050000000895_BODY"><br><br>
> + coio_read_xrow(coio, ibuf, &row);<br>
> + else {<br>
> + coio_read_xrow_timeout_xc(coio, ibuf, &row,<br>
> + replication_disconnect_timeout);<br>
> + }<br><br>
> diff --git a/test/replication/errinj.result b/test/replication/errinj.result<br>
> index d1f1dbe91..d0121e209 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 by default proportional to replication_timeout, so since it's very low<br>
> +-- for this test case, we should manually set it to new value to allow connection. It should be higher<br>
> +-- than master's replication_timeout (0.1 in this case)<br>
> +box.cfg{replication_disconnect_timeout = 1.}<br>
> +---<br>
> +...<br>
> +fiber = require('fiber')<br>
> +---<br>
> +...<br>
> +-- wait master's replication_timeout (0.1) couple times to make sure connection is done (follow)<br>
> +fiber.sleep(0.5)<br>
> +---<br>
> +...<br><br>
This timeout is too long. Please try to make it shorter so that the<br>
tests run faster.<br></div></div></div></div></blockquote>
Reduce it to 0.15s. Still even original test was quite long to complete..<br>
<br>С уважением,<br>Konstantin Belyavskiy<br>k.belyavskiy@tarantool.org<br></BODY></HTML>