Anton, how do you plan to rollback the transaction from a remote filter?
Are you planning to call setRollbackOnly()? Calling rollback() or commit()
from any filter or listener should not be allowed.
D.
On Tue, Jul 3, 2018 at 1:55 AM, Anton Vinogradov <a...@apache.org> wrote:
> Dmitriy, Yakov,
>
> I've finalized design and prepared the solution [1].
>
> 1) Only events from GridNearTxLocal are registered now.
>
> List 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
> };
> 2) Transaction can be rolled back now inside
> - remote filter (always, since it always happens on node started this
> transaction)
> - local listener (only at node started this transaction)
>
> Rollback uses rollbackOnlyProxy [2] specially designed (and tested) to
> rollback any tx from any thread at node started the transaction.
>
> I see another public tools doing the same:
> - TransactionsMXBean#getActiveTransactions
> - control.sh --tx kill Xid
>
> Both able to rollback any tx at any state remotely.
>
> Yakov,
> could you please review the code?
>
> [1] https://github.com/apache/ignite/pull/4036 (TC checked)
> [2]
> org.apache.ignite.internal.processors.cache.distributed.
> near.GridNearTxLocal#rollbackOnlyProxy
>
> пт, 1 июн. 2018 г. в 23:33, Dmitriy Setrakyan <dsetrak...@apache.org>:
>
> > Anton, we are very far from agreement. I think it makes sense to step
> back,
> > come up with a clean design and propose it again.
> >
> > On Fri, Jun 1, 2018 at 12:59 PM, Anton Vinogradov <a...@apache.org> wrote:
> >
> > > Dmitriy,
> > >
> > > Unfortunately, we have more than 2 types of txs, full list is
> > >
> > > GridDhtTxLocal
> > > GridDhtTxRemote
> > > GridNearTxLocal
> > > GridNearTxRemote
> > >
> > > BTW, We have no clear documentation about behaviour and difference.
> > > I created an issue [1] to solve this, but seems no one interested :(
> > >
> >
> > The reason we do not have a documentation for these transaction classes
> is
> > because they are part of the internal implementation and should never be
> > exposed to users.
> >
> > 1) What I see is that every Grid*Tx* have xid, startTime, isolation,
> > > concurrency, etc. So, there is no difference in params.
> > > Label is the only one exception to the rule, but this can be fixed.
> > >
> >
> > I am putting myself in the user's shoes, and no user will ever understand
> > what is the difference between all these internal transaction classes in
> > Ignite. Moreover, if you support events for all these transactions types,
> > then users will start getting duplicate events of identical types for the
> > same XID.
> >
> > As a user, all I care about is GridNearTxLocal events. On top of that, I
> do
> > not even care to know that internally Ignite calls it this way. All I
> care
> > about is the transaction events.
> >
> > So, every Grid*Tx* can provide it's params once it's state changed.
> > > And it's a good Idea to have possibility to see state changes and tx
> > > params.
> > >
> >
> > I am against exposing private transaction details or classes or events
> via
> > public API. Moreover, I do not see any value for users to know about
> > existence of these transaction classes, as they are part of the internal
> > implementation.
> >
> >
> > > 2) Other good idea is to have chance to rollback or resume or even
> commit
> > > transaction inside tx event listener on each state change.
> > > Currently "actions" available only from GridNearTxLocal events, but
> > what's
> > > the problem to allow rollback from GridDhtTxLocal in future?
> > >
> >
> > Actions like commit or rollback should *NEVER* be available from any
> event.
> > Why do you suggest they are available?
> >
> >
> > > 3) Currently, each TransactionStateChangedEvent provides "Transaction
> tx"
> > > which is special proxy for specific type of IgniteTxAdapter.
> > > GridNearTxLocal's proxy is fully implemented, other implemented
> > partially,
> > > but can be improved later.
> > >
> >
> > Disagree for the same reasons as stated above.
> >
> >
> > > What about to add flags 'local', 'near', 'dht' and 'remote' to
> > > TransactionStateChangedEvent to explain where state changed?
> > > In case it's 'near | local' you'll have chances to rollback such tx.
> > > In case it's 'dht | remote' you'll see what is the real last mile
> timeout
> > > for txs at your system. It can be much smaller than initial tx timeout.
> > >
> >
> > Disagree for the same reasons as stated above.
> >
> > 4) So, I propose to keep this design because it's good for *monitoring
> and
> > > restrictions* and improve it on demand (no refactoring needed, just
> > > implement cases throwing UnsupportedOperationException)
> > > - new EVT_TX_* events can be added, eg. EVT_TX_PREPARING
> > > - label can be shared to all txs (thats not a problem as I can see,
> and I
> > > can do it as a separate task)
> > > - rollback can be implemented on all Grid*TxLocal or even at
> > Grid*TxRemote
> > > :)
> > >
> >
> > It is completely wrong design to have any kind of transaction commit or
> > rollback action from inside of any event.
> >
> >
> > >
> > > [1] https://issues.apache.org/jira/browse/IGNITE-8419
> > >
> > > пт, 1 июн. 2018 г. в 19:35, Dmitriy Setrakyan <dsetrak...@apache.org>:
> > >
> > > > I do not like the inconsistent behavior between different transaction
> > > > events. I now feel that we need to separate events between Near TX
> and
> > > > Remote TX, and maybe focus on the Near TX for now.
> > > >
> > > > How about we only add events for the Near TX and have a consistent
> > > behavior
> > > > across all Near TX events. I would suggest that you rename your event
> > to
> > > > EVT_TX_NEAR_STARTED/PREPARED/COMMITTED/etc/etc? In this case the
> > > "label()"
> > > > method will always provide required data, right?
> > > >
> > > > D.
> > > >
> > > > On Fri, Jun 1, 2018 at 7:19 AM, Anton Vinogradov <a...@apache.org>
> > wrote:
> > > >
> > > > > Dmitriy,
> > > > >
> > > > > 1) EVT_TX_PREPARED were added this morning to check event
> generation
> > on
> > > > > remote nodes :)
> > > > >
> > > > > 2) Only GridNearTxLocal has label now, that's the implementation we
> > > > > currently have. It can be improved if necesary, I think.
> > > > > So, actually, label always available at
> > > > > - EVT_TX_STARTED,
> > > > > - EVT_TX_SUSPENDED,
> > > > > - EVT_TX_RESUMED
> > > > > since they can be fired only from originating node (from
> > > GridNearTxLocal)
> > > > >
> > > > > In case any other event will be fired by GridNearTxLocal it will
> > > contain
> > > > > label too.
> > > > > In case of user call label on remote event it will gain
> > > > > UnsupportedOperationException.
> > > > >
> > > > > BTW, rollback also available only at events produced by
> > > GridNearTxLocal.
> > > > >
> > > > > пт, 1 июн. 2018 г. в 16:29, Dmitriy Setrakyan <
> dsetrak...@apache.org
> > >:
> > > > >
> > > > > > Ok, sounds good.
> > > > > >
> > > > > > I till have more comments:
> > > > > >
> > > > > > 1. I think you have missed EVT_TX_PREPARED event
> > > > > > 2. I am still very confused with your comment on "label()"
> > method.
> > > > Why
> > > > > > is the label not propagated to remote nodes? What happens when
> > > users
> > > > > > call
> > > > > > this "label()" method for other TX events, not the
> > EVT_TX_STARTED
> > > > > event?
> > > > > >
> > > > > > D.
> > > > > >
> > > > > > On Fri, Jun 1, 2018 at 6:20 AM, Anton Vinogradov <a...@apache.org>
> > > > wrote:
> > > > > >
> > > > > > > Dmitriy,
> > > > > > >
> > > > > > > In that case there will be no chances to listen only tx
> creation
> > > > events
> > > > > > > without slowing down the system on other tx events creation and
> > > > > > filtering.
> > > > > > > All events are processed at same thread where tx changes the
> > state,
> > > > so,
> > > > > > we
> > > > > > > have to have the way to decrease potential slowdown.
> > > > > > >
> > > > > > > I made it similar to
> > > > > > > public static final int[] EVTS_CACHE = {
> > > > > > > EVT_CACHE_ENTRY_CREATED,
> > > > > > > EVT_CACHE_ENTRY_DESTROYED,
> > > > > > > EVT_CACHE_OBJECT_PUT,
> > > > > > > EVT_CACHE_OBJECT_READ,
> > > > > > > EVT_CACHE_OBJECT_REMOVED,
> > > > > > > EVT_CACHE_OBJECT_LOCKED,
> > > > > > > EVT_CACHE_OBJECT_UNLOCKED,
> > > > > > > EVT_CACHE_OBJECT_EXPIRED
> > > > > > > };
> > > > > > >
> > > > > > >
> > > > > > > чт, 31 мая 2018 г. в 20:48, Dmitriy Setrakyan <
> > > dsetrak...@apache.org
> > > > >:
> > > > > > >
> > > > > > > > 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.
> > > > > > > > > > > > > >
> > > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
