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 16:34:26 GMT
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