Re: [tarantool-patches] [PATCH v3 11/14] txn: introduce asynchronous txn commit

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

On Thu, Jun 13, 2019 at 10:45:18PM +0300, Георгий Кириченко wrote:
> On Thursday, June 13, 2019 5:34:00 PM MSK Vladimir Davydov wrote:
> > On Sun, Jun 09, 2019 at 11:44:40PM +0300, Georgy Kirichenko wrote:
> > > diff --git a/src/box/txn.c b/src/box/txn.c
> 
> > I don't see why we need to have in_txn() in on_commit/rollback triggers.
> > Could you please point me?
> Unfortunately lua on_commit triggers use it (because of vshard requirements) 
> and I'm don't see the right way to remove it.

Okay, I see. Let's add a comment please that this is needed for Lua
triggers. Just curious, how can it possibly use it?

> > 
> > > diff --git a/src/box/wal.c b/src/box/wal.c
> > > index e868a8e71..eff48b4fe 100644
> > > --- a/src/box/wal.c
> > > +++ b/src/box/wal.c
> > > @@ -272,6 +272,8 @@ tx_schedule_f(va_list ap)
> > > 
> > >  			struct journal_entry *req =
> > >  			
> > >  				stailq_shift_entry(&writer->schedule_queue,
> > >  				
> > >  						   struct journal_entry, fifo);
> > > 
> > > +			if (req->on_done_cb != NULL)
> > > +				req->on_done_cb(req, req->on_done_cb_data);
> > > 
> > >  			req->done = true;
> > >  			fiber_cond_broadcast(&req->done_cond);
> > 
> > Why do we need cond if we have a callback? Can't we wake up the awaiting
> > fiber from the callback?
> The condition and the done variable is required the time when all transaction 
> data could be purged.

I think we could purge async transactions right in the callback.

> Definitely it could not be done while on_done_cb because 
> we will lost transaction status in this case. I think there are some ways to 
> handle this but I think the solution would be too complicated.

I don't get what you mean by "loosing transaction status". Could you
elaborate please?

> I use the callback to process txn finalization and call on_commit/on_rollback 
> trigger one of them signals an applier fiber to collect transaction and 
> determine replication state.

Why can't we collect the transaction right from the completion callback?
I think we could propage a failure to applier via on_rollback trigger.
I something wrong with it?