Re: [tarantool-patches] [PATCH v6 3/3] box: adds replication sync after cfg. update

[Vladimir Davydov <vdavydov.dev@gmail.com>]

On Thu, Aug 30, 2018 at 05:11:14PM +0300, Olga Arkhangelskaia wrote:
> diff --git a/src/box/box.cc b/src/box/box.cc
> index dcedfd002..e54a79467 100644
> --- a/src/box/box.cc
> +++ b/src/box/box.cc
> @@ -646,6 +646,12 @@ box_set_replication(void)
>  	box_sync_replication(true);
>  	/* Follow replica */
>  	replicaset_follow();
> +	/* Set orphan and sync replica up to quorum.
> +	 * If we fail to sync up, replica will be left in orphan state.
> +	 */
> +	is_orphan = true;
> +	title("orphan");

It isn't enough. You should also wake up fibers waiting on ro_cond, take
a look at box_clear_orphan(). Also, I think that it's worth
encapsulating this code in function box_set_orphan().

> +	replicaset_sync();

If the instance happens to be synchronized when this function is called,
it will still enter orphan state briefly and then leave it in a while.
While in orphan state, it might reject some rw requests. I asked Kostja
and Georgy about that. Both of them agreed that it isn't good. Ideally
we shouldn't enter orphan state, even briefly, if the instance doesn't
need to wait for appliers to synchronize. In other words, if vclock
received by at least replication_connect_quorum appliers in reply to
SUBSCRIBE command is less than replicaset.vclock we shouldn't enter
orphan state.

May be, I'm missing something. You might want to talk to Kostja and/or
Georgy about that.

>  }
>  
>  void
> diff --git a/src/box/replication.cc b/src/box/replication.cc
> index be58b0225..d85700b78 100644
> --- a/src/box/replication.cc
> +++ b/src/box/replication.cc
> @@ -700,6 +700,7 @@ replicaset_sync(void)
>  
>  	say_crit("replica set sync complete, quorum of %d "
>  		 "replicas formed", quorum);
> +	return;

Pointless change.

>  }
>  
>  void
> diff --git a/src/box/replication.h b/src/box/replication.h
> index a6f1dbf69..64f6e7f97 100644
> --- a/src/box/replication.h
> +++ b/src/box/replication.h
> @@ -378,10 +378,9 @@ void
>  replicaset_follow(void);
>  
>  /**
> - * Wait until a replication quorum is formed.
> - * Return immediately if a quorum cannot be
> - * formed because of errors.
> + * Wait until a replication quorum is formed and sync up with it.
>   */
> +

Why did you change the comment?

>  void
>  replicaset_sync(void);
>  

> diff --git a/test/replication/sync.test.lua b/test/replication/sync.test.lua

Tests didn't pass on Travis CI:

https://travis-ci.org/tarantool/tarantool/builds/422570554?utm_source=github_status&utm_medium=notification

> new file mode 100644
> index 000000000..0c4fff483
> --- /dev/null
> +++ b/test/replication/sync.test.lua
> @@ -0,0 +1,71 @@
> +fiber = require('fiber')
> +--
> +-- gh-3427: no sync after configuration update
> +--
> +
> +--
> +-- successful sync
> +--
> +
> +env = require('test_run')
> +test_run = env.new()
> +engine = test_run:get_cfg('engine')
> +
> +box.schema.user.grant('guest', 'replication')
> +
> +test_run:cmd("create server replica with rpl_master=default, script='replication/replica.lua'")
> +test_run:cmd("start server replica")
> +
> +s = box.schema.space.create('test', {engine = engine})
> +index = s:create_index('primary')
> +
> +-- change replica configuration
> +test_run:cmd("switch replica")
> +replication = box.cfg.replication
> +box.cfg{replication={}}
> +
> +test_run:cmd("switch default")
> +-- insert values on the master while replica is unconfigured
> +box.begin() for i = 1, 100 do box.space.test:insert{i, i} end box.commit()
> +box.space.test:count()
> +
> +test_run:cmd("switch replica")
> +box.cfg{replication = replication}
> +box.space.test:count() == 100
> +
> +--
> +-- unsuccessful sync entering orphan state
> +--
> +box.cfg{replication={}}
> +box.cfg{replication_sync_timeout = 0.000001}

This looks flimsy. Should test-run stall for a millisecond, and the test
would fail. I'd use error injections to make sure it doesn't happen.

> +
> +test_run:cmd("switch default")
> +-- insert values on the master while replica is unconfigured
> +box.begin() for i = 101, 200 do box.space.test:insert{i, i} end box.commit()
> +
> +test_run:cmd("switch replica")
> +box.cfg{replication = replication}
> +box.info.status
> +require'fiber'.sleep(0.1)
> +box.info.status
> +
> +--
> +-- replication_sync_lag is too big
> +--
> +
> +box.cfg{replication_sync_lag = 100}

If I change the lag to 1 or 0.1 or 0.0000001, the test still passes,
i.e. it doesn't check that this option actually works.

> +
> +test_run:cmd("switch default")
> +
> +function f () box.begin() for i = 201, 500 do box.space.test:insert{i, i} end box.commit(); end

These numbers (100, 200, 500) look random. Using error injection would
let you make them look reasonable.

> +_=fiber.create(f)
> +
> +test_run:cmd("switch replica")
> +box.space.test:count() < 500
> +
> +test_run:cmd("switch default")
> +-- cleanup
> +test_run:cmd("stop server replica")
> +test_run:cmd("cleanup server replica")
> +box.space.test:drop()
> +box.schema.user.revoke('guest', 'replication')

You don't test that the instance enters orphan mode and that it
eventually leaves it.