Re: [tarantool-patches] Re: [PATCH v4 8/9] applier: apply transaction in parallel

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

On Thu, Jun 20, 2019 at 11:33:50PM +0300, Георгий Кириченко wrote:
> On Thursday, June 20, 2019 7:37:09 PM MSK Vladimir Davydov wrote:
> > On Thu, Jun 20, 2019 at 10:41:10AM +0300, Георгий Кириченко wrote:
> > > I'm sorry, there is proper version of the commit:
> > > 
> > > Applier use asynchronous transaction to batch journal writes. All
> > > appliers share the replicaset.applier.tx_vclock which means the vclock
> > > applied but not necessarily written to a journal. Appliers use a trigger
> > > to coordinate in case of failure - when a transaction is going to
> > > be rolled back. Also an applier writer condition is shared across all
> > > appliers and signaled in case of commit or hearth beat message.
> > > 
> > > Closes: #1254
> > > ---
> > > 
> > >  src/box/applier.cc     | 156 +++++++++++++++++++++++++++++------------
> > >  src/box/applier.h      |   9 ++-
> > >  src/box/replication.cc |   7 ++
> > >  src/box/replication.h  |  14 ++++
> > >  4 files changed, 138 insertions(+), 48 deletions(-)
> > > 
> > > diff --git a/src/box/applier.cc b/src/box/applier.cc
> > > index 5a92f6109..fee49d8ca 100644
> > > --- a/src/box/applier.cc
> > > +++ b/src/box/applier.cc
> > > @@ -50,6 +50,7 @@
> > > 
> > >  #include "schema.h"
> > >  #include "txn.h"
> > >  #include "box.h"
> > > 
> > > +#include "scoped_guard.h"
> > > 
> > >  STRS(applier_state, applier_STATE);
> > > 
> > > @@ -130,11 +131,24 @@ applier_writer_f(va_list ap)
> > > 
> > >  		 * replication_timeout seconds any more.
> > >  		 */
> > >  		
> > >  		if (applier->version_id >= version_id(1, 7, 7))
> > > 
> > > -			fiber_cond_wait_timeout(&applier->writer_cond,
> > > +			fiber_cond_wait_timeout(&replicaset.applier.commit_cond,
> > > 
> > >  						TIMEOUT_INFINITY);
> > >  		
> > >  		else
> > > 
> > > -			fiber_cond_wait_timeout(&applier->writer_cond,
> > > +			fiber_cond_wait_timeout(&replicaset.applier.commit_cond,
> > > 
> > >  						replication_timeout);
> > 
> > Why replace applier->writer_cond with replicaset.applier.commit_cond?
> > This means that even if only one applier is active, we will wake up all
> > of the writers on each commit, which looks strange.
> I did it because an applier doesn't have any control of how transaction is 
> finished except an on_commit/on_rollback trigger.

Okay, we can wake up the appropriate applier from the trigger, can we?

> However if an applier sends nothing to commit (for instance it could
> be behind others) it still should send ACK.

In which case we can wake up the applier from applier_apply_tx.

> Also I think we should update this state for any transaction processed
> (even for local ones).

This I don't understand.

> > 
> > > +		/*
> > > +		 * Stay 'orphan' until appliers catch up with
> > > +		 * the remote vclock at the time of SUBSCRIBE
> > > +		 * and the lag is less than configured.
> > > +		 */
> > > +		if (applier->state == APPLIER_SYNC &&
> > > +		    applier->lag <= replication_sync_lag &&
> > > +		    vclock_compare(&applier->remote_vclock_at_subscribe,
> > > +				   &replicaset.vclock) <= 0) {
> > > +			/* Applier is synced, switch to "follow". */
> > > +			applier_set_state(applier, APPLIER_FOLLOW);
> > > +		}
> > > +
> > 
> > A writer is supposed to send ACKs, not change the applier state.
> > How did this wind up here? Can't we do this right from the on_commit
> > trigger?
> The same case above - if applier didn't send anything to commit (it is behind 
> other applier) where is the better point to update its state.

In applier_apply_tx or applier_subscribe?

Just that it looks really weird that the writer fiber, the sole purpose
of which is to reply with ACKs, can also update the applier state.

> > > @@ -735,6 +820,15 @@ applier_subscribe(struct applier *applier)
> > > 
> > >  	applier->lag = TIMEOUT_INFINITY;
> > > 
> > > +	/* Register a trigger to handle replication failures. */
> > > +	struct trigger on_fail;
> > > +	trigger_create(&on_fail, applier_on_fail, applier, NULL);
> > > +	trigger_add(&replicaset.applier.on_replication_fail, &on_fail);
> > 
> > Why do we need on_replication_fail trigger? AFAICS it is called from
> > on_rollback callback. Can't we call applier_on_fail right from there,
> > without the use of the intermediary?
> Because we should cancel all appliers if anything failed (for instance an 
> applier could skip a transaction and start with the next one and then should 
> be cancelled if other applier failed to). We could track the applier list but 
> I'm not sure it would be better.

We didn't cancel all appliers before and it worked just fine so I fail
to understand why we need to do it now. Could you please give an example
when something breaks because of that?

> > > diff --git a/src/box/replication.h b/src/box/replication.h
> > > index 8c8a9927e..a4830f5b5 100644
> > > --- a/src/box/replication.h
> > > +++ b/src/box/replication.h
> > > @@ -232,6 +232,20 @@ struct replicaset {
> > > 
> > >  		 * struct replica object).
> > >  		 */
> > >  		
> > >  		struct latch order_latch;
> > > 
> > > +		/*
> > > +		 * A vclock of the last transaction wich was read
> > > +		 * from an applier connection.
> > > +		 */
> > > +		struct vclock net_vclock;
> > 
> > Please elaborate. Can it be less than replicaset.vclock? Can it be
> > greater? Why?
> Let discuss it f2f.

I just want the comment improved :)