ignite-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Anton Vinogradov ...@apache.org>
Subject Re: Ability to check and completely fill transactions on creation
Date Mon, 09 Jul 2018 11:25:13 GMT
Dmitriy,

Rollback from remote filter uses rollbackOnlyProxy [1], that's a special
proxy allows rollback from another thread.
It was specially designed to rollback transactions by "label" if necessary.
So, I'm just finishing "label feature" to make it more useful at real
production.

Here's the example of remote filter with rollback (more examples can be
found at PR [2])

ignite.events().remoteListen(null,
(IgnitePredicate<Event>)e -> {
    TransactionStateChangedEvent evt = (TransactionStateChangedEvent)e;
Transaction tx = evt.tx();
if (tx.label() == null) // Timeout and orher details can be checked as well.
tx.rollback();
return true;
}, EVT_TX_STARTED);

>> Calling rollback() or commit() from any filter or listener should not be
allowed.
Only rollback allowed, but reasonable question is "Why?", now I see we
already doing this at:
- TransactionsMXBean#getActiveTransactions -> foreach.rollback
- control.sh --tx kill Xid

The only one difference is that filter or listener can validate or/and
rollback any tx synchronously, before it breaks something (eg. on start or
resume),
while TransactionsMXBean#getActiveTransactions or control.sh do this in
batch way without any sync warranty.


[1]
org.apache.ignite.internal.processors.cache.distributed.near.GridNearTxLocal#rollbackOnlyProxy
[2] https://github.com/apache/ignite/pull/4036

пн, 9 июл. 2018 г. в 7:04, Dmitriy Setrakyan <dsetrakyan@apache.org>:

> 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 <av@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 <dsetrakyan@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 <av@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 <dsetrakyan@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 <av@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 <
> > dsetrakyan@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 <
> av@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
<
> > > > dsetrakyan@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
<
> > > av@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 <
> > > > > > dsetrakyan@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 <
> > > > > av@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 <
> > > > > > > > dsetrakyan@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 <
> > > > > > > av@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 <
> > > > > > > > > > dsetrakyan@apache.org
> > > > > > > > > > > >:
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > > On Wed, May 23,
2018 at 4:08 AM, Anton
> > Vinogradov <
> > > > > > > > > av@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.
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > >
> > > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message