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