Anton,
Why not just have one transaction event: EVT_TX_STATE_CHANGED?
D.
On Thu, May 31, 2018 at 9:10 AM, Anton Vinogradov <a...@apache.org> wrote:
> Dmitriy,
>
> Thanks for your comments!
>
> I've updated design to have
>
> public class TransactionStateChangedEvent extends EventAdapter {
> private Transaction tx;
> }
>
> also I specified following set of possible events
>
> public static final int[] EVTS_TX = {
> EVT_TX_STARTED,
> EVT_TX_COMMITTED,
> EVT_TX_ROLLED_BACK,
> EVT_TX_SUSPENDED,
> EVT_TX_RESUMED
> };
>
> It contains most of reasonable tx states changes.
> Additional events can be added later if necessary.
>
>
> Tx label() available only at EVT_TX_STARTED because it is not propagated to
> remote nodes, but
>
> - xid()
> - nodeId()
> - threadId()
> - startTime()
> - isolation()
> - concurrency()
> - implicit()
> - isInvalidate()
> - state()
> - timeout()
>
> now available at any tx state change event.
>
>
> As usual, full code listing available at
> https://github.com/apache/ignite/pull/4036/files
>
>
> вт, 29 мая 2018 г. в 20:41, Dmitriy Setrakyan <dsetrak...@apache.org>:
>
> > Anton,
> >
> > We cannot have TransactionStartedEvent without having events for all
> other
> > transaction states, like TransactionPreparedEvent,
> > TransactionCommittedEvent, etc. Considering this, I sill do not like the
> > design, as we would have to create many extra event classes.
> >
> > Instead, I would suggest that you create TransactionStateChangeEvent,
> which
> > would have previous and new transaction state and would cover all state
> > changes, not just the start of the transaction. This will make the design
> > consistent and thorough.
> >
> > D.
> >
> > On Tue, May 29, 2018 at 5:39 AM, Anton Vinogradov <a...@apache.org> wrote:
> >
> > > Dmitriy,
> > > I fixed design according to your and Yakov's comments, thanks again for
> > > clear explanation.
> > >
> > > >> 1. You use internal API in public event, i.e. you cannot have user
> > > >> accessing to IgniteInternalTx instance through TxEvent.
> > >
> > > Event definition changed to
> > > public class TransactionStartedEvent extends EventAdapter {
> > > private IgniteTransactions tx;
> > > ...
> > > }
> > >
> > > Not it's 100% public.
> > >
> > > >> 2. Throwing runtime errors from listener is not documented and I
> doubt
> > > if
> > > >> it can be fully supported in the pattern you use in TxLabelTest.
> After
> > > >> looking at the mentioned test user may think that throwing runtime
> > error
> > > >> when notified on new node join may prohibit new node joining which
> is
> > > not
> > > >> true. Do you have any example in Ignite when throwing exception from
> > > >> listener is valid and documented.
> > >
> > > Test's logic changed to
> > > ...
> > > // Label
> > > IgniteTransactions tx = evt.tx();
> > >
> > > if (tx.label() == null)
> > > tx.tx().rollback();
> > > ...
> > > and
> > > ...
> > > // Timeout
> > > Transaction tx = evt.tx().tx();
> > >
> > > if (tx.timeout() < 200)
> > > tx.rollback();
> > > ...
> > >
> > > So, tx will be rollbacked on creation and any commit attempt will cause
> > > TransactionRollbackException
> > >
> > > Full code listing available at
> > > https://github.com/apache/ignite/pull/4036/files
> > >
> > > Dmitriy, Yakov,
> > > Could you please check and confirm changes?
> > >
> > > чт, 24 мая 2018 г. в 16:32, Dmitriy Setrakyan <dsetrak...@apache.org>:
> > >
> > > > Anton, why do you need to *alter* event sub-system to introduce a new
> > > > event? Yakov's issue was that you propagated private interface to
> > public
> > > > API, which is bad of course. Come up with a clean design and it will
> be
> > > > accepted.
> > > >
> > > > My problem with TransactionValidator is that it only solves a small
> > > problem
> > > > for transactions. If we do that, then we will have to add cache
> > > validators,
> > > > compute validators, etc, etc, etc. That is why we either should use
> the
> > > > existing event subsystem or come up with a holistic design that will
> > work
> > > > across the whole project.
> > > >
> > > > D.
> > > >
> > > > On Thu, May 24, 2018 at 1:38 AM, Anton Vinogradov <a...@apache.org>
> > wrote:
> > > >
> > > > > Dmitriy,
> > > > >
> > > > > Yakov is against the solution based on event sub-system
> > > > > >> I think that we should think about some other solution instead
> of
> > > > > altering
> > > > > >> event sub-system.
> > > > >
> > > > > Also, I checked is there any chances to fix all the issues found by
> > > Yakov
> > > > > and see that solution becomes overcomplicated in that case.
> > > > > That's why I'm proposing this lightweight solution.
> > > > >
> > > > > As for me it's a good idea to have such validator since that's a
> > common
> > > > > problem at huge deployments when more than one team have access to
> > > Ignite
> > > > > cluster and there is no other way to setup tx cretion rules.
> > > > >
> > > > > Yakov,
> > > > >
> > > > > Could you please share your thoughts on that?
> > > > >
> > > > >
> > > > > чт, 24 мая 2018 г. в 8:58, Dmitriy Setrakyan <
> dsetrak...@apache.org
> > >:
> > > > >
> > > > > > On Wed, May 23, 2018 at 4:08 AM, Anton Vinogradov <a...@apache.org
> >
> > > > wrote:
> > > > > >
> > > > > > > Dmitriy, Yakov
> > > > > > >
> > > > > > > Are there any objections to updated design taking into account
> > the
> > > > > > comments
> > > > > > > I provided?
> > > > > > >
> > > > > >
> > > > > > Anton, I do not like an additional validator. I think you can
> > > > accomplish
> > > > > > the same with a transaction event. You just need to design it
> more
> > > > > cleanly,
> > > > > > incorporating the feedback from Yakov.
> > > > > >
> > > > >
> > > >
> > >
> >
>
