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 Tue, 03 Jul 2018 08:55:43 GMT
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