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.
> > > > >
> > > >
> > >
> >
>
