[tarantool-patches] Re: [PATCH v4 8/9] applier: apply transaction in parallel
Vladimir Davydov
vdavydov.dev at gmail.com
Fri Jun 21 11:36:26 MSK 2019
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 :)
More information about the Tarantool-patches
mailing list