<div dir="ltr">Thanks for the review. Will fix.</div><br><div class="gmail_quote"><div dir="ltr">чт, 23 авг. 2018 г. в 18:06, Vladimir Davydov <<a href="mailto:vdavydov.dev@gmail.com">vdavydov.dev@gmail.com</a>>:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On Wed, Aug 22, 2018 at 06:59:30PM +0300, Olga Arkhangelskaia wrote:<br>
> When replica reconnects to replica set not for the first time, we<br>
> suffer from absence of synchronization. Such behavior leads to giving<br>
> away outdated data.<br>
> <br>
> Closes #3427<br>
> ---<br>
> <a href="https://github.com/tarantool/tarantool/issues/3427" rel="noreferrer" target="_blank">https://github.com/tarantool/tarantool/issues/3427</a><br>
> <a href="https://github.com/tarantool/tarantool/tree/OKriw/replication_no_sync-1.9" rel="noreferrer" target="_blank">https://github.com/tarantool/tarantool/tree/OKriw/replication_no_sync-1.9</a><br>
> <br>
> v1:<br>
> <a href="https://www.freelists.org/post/tarantool-patches/PATCH-replication-adds-replication-sync-after-cfg-update" rel="noreferrer" target="_blank">https://www.freelists.org/post/tarantool-patches/PATCH-replication-adds-replication-sync-after-cfg-update</a><br>
> <br>
> Changes in v2:<br>
> - fixed test<br>
> - changed replicaset_sync<br>
> <br>
>  src/box/box.cc                      |  7 ++-<br>
>  src/box/replication.cc              | 34 ++++++++------<br>
>  src/box/replication.h               |  7 ++-<br>
>  test/replication/orphan.result      | 92 +++++++++++++++++++++++++++++++++++++<br>
>  test/replication/orphan.test.lua    | 41 +++++++++++++++++<br>
>  test/replication/replica_orphan.lua | 12 +++++<br>
<br>
The test isn't about the 'orphan' state. It's about replica set<br>
synchronization. Please call it appropriately.<br>
<br>
Also, the test takes prohibitively long - 15 seconds for memtx+vinyl.<br>
Please try to reduce the test run time.<br>
<br>
Also, the test passes with and without the changes done to the code, in<br>
other words it's useless. Please make sure, it doesn't pass without your<br>
patch.<br>
<br>
<br>
>  6 files changed, 177 insertions(+), 16 deletions(-)<br>
>  create mode 100644 test/replication/orphan.result<br>
>  create mode 100644 test/replication/orphan.test.lua<br>
>  create mode 100644 test/replication/replica_orphan.lua<br>
> <br>
> diff --git a/src/box/box.cc b/src/box/box.cc<br>
> index 8d7454d1f..8c67c79e8 100644<br>
> --- a/src/box/box.cc<br>
> +++ b/src/box/box.cc<br>
> @@ -634,6 +634,9 @@ box_set_replication(void)<br>
>       box_sync_replication(true);<br>
>       /* Follow replica */<br>
>       replicaset_follow();<br>
> +     /* Sync replica up to quorum */<br>
> +     replicaset_sync();<br>
> +     say_verbose("synchronization complete");<br>
<br>
This message is pointless IMO. I think that replication reconfiguration<br>
should throw an error if it failed to synchronize with the quorum. It<br>
would be consistent with the fact that it throws an error in case it<br>
failed to connect to the quorum.<br>
<br>
>  }<br>
>  <br>
>  void<br>
> @@ -1941,8 +1944,10 @@ box_cfg_xc(void)<br>
>       fiber_gc();<br>
>       is_box_configured = true;<br>
>  <br>
> -     if (!is_bootstrap_leader)<br>
> +     if (!is_bootstrap_leader) {<br>
>               replicaset_sync();<br>
> +             replicaset_is_synced();<br>
> +     }<br>
<br>
Instead of introducing another function, you could add a return value to<br>
replicaset_sync() - true in case it successfully synchronized, false<br>
otherwise - then use the return value to either log or throw an error,<br>
depending whether its initial configuration or reconfiguration. IMO it<br>
would look neater.<br>
</blockquote></div>