ignite-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Dmitriy Setrakyan <dsetrak...@apache.org>
Subject Re: Ability to check and completely fill transactions on creation
Date Fri, 01 Jun 2018 20:32:46 GMT
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