From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Fri, 21 Jun 2019 11:36:26 +0300 From: Vladimir Davydov Subject: Re: [tarantool-patches] Re: [PATCH v4 8/9] applier: apply transaction in parallel Message-ID: <20190621083626.aosoqt523sgii6o4@esperanza> References: <4226262.a1cURWbef2@home.lan> <20190620163709.6hlxnsyekyouygde@esperanza> <3160961.t2HJ1z8aLX@home.lan> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <3160961.t2HJ1z8aLX@home.lan> To: =?utf-8?B?0JPQtdC+0YDQs9C40Lkg0JrQuNGA0LjRh9C10L3QutC+?= Cc: tarantool-patches@freelists.org List-ID: 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 :)