You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@ignite.apache.org by Anton Vinogradov <av...@apache.org> on 2018/05/21 10:18:20 UTC

Re: Ability to check and completely fill transactions on creation

Igniters,

Fix is ready.
Code checked by Nikolay Izhikov, all issues fixed
Code benchmarked by Ilya Suntsov, no drop found.

Fix *affects public API*, so I'm proposing everyone interested check it.
Issue: https://issues.apache.org/jira/browse/IGNITE-8446
PR: https://github.com/apache/ignite/pull/4036/files

Public API changes:
New EvenType:

EVT_TX_STARTED = 129.

New Event:

TransactionEvent extends EventAdapter {
   private IgniteInternalTx tx;
}

In case there are no objection I'll merge the changes soon.

пт, 6 апр. 2018 г. в 14:12, Anton Vinogradov <av...@apache.org>:

> >> But I have concern
> >> about performance. How can you estimate impact to performance ?
> We have to benchmark result.
>
> >> How about to set label name with some useful info if user does not
> provide
> >> custom name?
> You can set custom listener which will do that
>
> >>  For example thread name + global counter?
> Or even full stacktrace
>
> >>  how the user is expected to use this event?
> Event will be used to validate tx on creation.
> Since listner will be invoked at same thread (Am I right?) it will have
> all requred info for validation.
>
>
> 2018-04-05 22:06 GMT+03:00 Denis Magda <dm...@apache.org>:
>
>> Guys,
>>
>> Sorry for a dumb question but how the user is expected to use this event?
>>
>> --
>> Denis
>>
>> On Thu, Apr 5, 2018 at 6:06 AM, Anton Vinogradov <av...@apache.org> wrote:
>>
>> > Igniters,
>> >
>> > As far as I know we're working on additional 'label' field for
>> transactions
>> > [1].
>> > That's great and will be helpful for customers with huge deploymens to
>> see
>> > reason of each transaction.
>> > But, since 'label' is optional field, there is no way to guarantee it
>> will
>> > be filled.
>> >
>> > I'd like to propose an idea of brand new event EVT_USR_TX_CREATED (local
>> > transaction created).
>> >
>> > Local listener on such event will allow to guarantee tx's 'label' field
>> > filled, timeout is correct and so on.
>> >
>> > Thoughts?
>> >
>> > [1] https://issues.apache.org/jira/browse/IGNITE-6827
>> >
>>
>
>

Re: Ability to check and completely fill transactions on creation

Posted by Anton Vinogradov <av...@apache.org>.
Dmitriy,

Yes, this achieves the same purpose, thanks for hint.
Updated PR to use strictly setRollbackOnly().

ср, 11 июл. 2018 г. в 4:17, Dmitriy Setrakyan <ds...@apache.org>:

> Anton,
>
> Committing or rolling back from MXBean is OK, because it is not a listener,
> but a direct invocation. However, it is not OK to allow synchronous
> rollback from a filter or a listener. The only action you can do from a
> listener is setRollbackOnly() which will cause the transaction to be rolled
> back eventually. I think it achieves the same purpose.
>
> D.
>
> On Mon, Jul 9, 2018 at 2:25 PM, Anton Vinogradov <av...@apache.org> wrote:
>
> > 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 <ds...@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.
> > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > >
> > > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>

Re: Ability to check and completely fill transactions on creation

Posted by Dmitriy Setrakyan <ds...@apache.org>.
Anton,

Committing or rolling back from MXBean is OK, because it is not a listener,
but a direct invocation. However, it is not OK to allow synchronous
rollback from a filter or a listener. The only action you can do from a
listener is setRollbackOnly() which will cause the transaction to be rolled
back eventually. I think it achieves the same purpose.

D.

On Mon, Jul 9, 2018 at 2:25 PM, Anton Vinogradov <av...@apache.org> wrote:

> 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 <ds...@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 <ds...@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.
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > >
> > > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>

Re: Ability to check and completely fill transactions on creation

Posted by Anton Vinogradov <av...@apache.org>.
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 <ds...@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 <ds...@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.
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > >
> > > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>

Re: Ability to check and completely fill transactions on creation

Posted by Dmitriy Setrakyan <ds...@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 <ds...@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 <ds...@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.
> > > > > > > > > > > > > >
> > > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>

Re: Ability to check and completely fill transactions on creation

Posted by Anton Vinogradov <av...@apache.org>.
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 <ds...@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 <ds...@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.
> > > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>

Re: Ability to check and completely fill transactions on creation

Posted by Dmitriy Setrakyan <ds...@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 <ds...@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 <ds...@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.
> > > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>

Re: Ability to check and completely fill transactions on creation

Posted by Anton Vinogradov <av...@apache.org>.
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 :(


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.

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.

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?

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.

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.
...

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
:)

[1] https://issues.apache.org/jira/browse/IGNITE-8419

пт, 1 июн. 2018 г. в 19:35, Dmitriy Setrakyan <ds...@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 <ds...@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.
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>

Re: Ability to check and completely fill transactions on creation

Posted by Dmitriy Setrakyan <ds...@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 <ds...@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 <ds...@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.
> > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>

Re: Ability to check and completely fill transactions on creation

Posted by Anton Vinogradov <av...@apache.org>.
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 <ds...@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 <ds...@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.
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>

Re: Ability to check and completely fill transactions on creation

Posted by Dmitriy Setrakyan <ds...@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 <ds...@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 <ds...@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.
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>

Re: Ability to check and completely fill transactions on creation

Posted by Anton Vinogradov <av...@apache.org>.
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 <ds...@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 <ds...@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.
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>

Re: Ability to check and completely fill transactions on creation

Posted by Dmitriy Setrakyan <ds...@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 <ds...@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 <ds...@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.
> > > > > >
> > > > >
> > > >
> > >
> >
>

Re: Ability to check and completely fill transactions on creation

Posted by Anton Vinogradov <av...@apache.org>.
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 <ds...@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 <ds...@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.
> > > > >
> > > >
> > >
> >
>

Re: Ability to check and completely fill transactions on creation

Posted by Dmitriy Setrakyan <ds...@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 <ds...@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 <ds...@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.
> > > >
> > >
> >
>

Re: Ability to check and completely fill transactions on creation

Posted by Anton Vinogradov <av...@apache.org>.
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 <ds...@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 <ds...@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.
> > >
> >
>

Re: Ability to check and completely fill transactions on creation

Posted by Dmitriy Setrakyan <ds...@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 <ds...@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.
> >
>

Re: Ability to check and completely fill transactions on creation

Posted by Anton Vinogradov <av...@apache.org>.
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 <ds...@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.
>

Re: Ability to check and completely fill transactions on creation

Posted by Dmitriy Setrakyan <ds...@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.

Re: Ability to check and completely fill transactions on creation

Posted by Anton Vinogradov <av...@apache.org>.
Dmitriy, Yakov

Are there any objections to updated design taking into account the comments
I provided?

пн, 21 мая 2018 г. в 18:49, Anton Vinogradov <av...@apache.org>:

> One more case is to analize and log tx's creators info without tx creation
> restriction.
> This is very important feature on huge system trial period when you may
> want to check who creates tx, tx content and configuration.
> For example you may want to log stacktrace for any tx without meta or with
> empty timeout.
> This will allow you to find a team responsible for that and make sure that
> they will fix their code.
>
> пн, 21 мая 2018 г. в 18:14, Anton Vinogradov <av...@apache.org>:
>
>> Dmitriy,
>>
>> Main idea is to restrict transaction creation in case label or timeout
>> are not set.
>> But there can be variations, for example label should fit some regexp or
>> timeout should be between 30 and 5000 ms.
>>
>> That's not easy to restrict this at user code when you have dozens of
>> libraries written by different people in one system using one ignite
>> cluster.
>> That solution based on idea of TopologyValidator when you have no chances
>> to use cluster in case something wrong with nodes.
>> So, any client should have no chances to create transaction not suitable
>> for this ignite cluster.
>>
>> пн, 21 мая 2018 г. в 17:48, Dmitriy Setrakyan <ds...@apache.org>:
>>
>>> Anton,
>>>
>>> The change looks very questionable. We cannot be adding configuration
>>> validators for every piece of Ignite API. What is it you are trying to
>>> achieve?
>>>
>>> D.
>>>
>>> On Mon, May 21, 2018 at 7:22 AM, Anton Vinogradov <av...@apache.org> wrote:
>>>
>>> > Yakov, thank's for deep check.
>>> >
>>> > >> I think that we should think about some other solution instead of
>>> > altering
>>> > >> event sub-system.
>>> >
>>> > Thank's to your comments now I see that solution is not perfect.
>>> >
>>> > How about to create
>>> >
>>> > interface TransactionsValidator {
>>> >    boolean validate(IgniteTransactions tx){
>>> >       ...
>>> >    }
>>> > }
>>> >
>>> > and add it's setter to IgniteConfiguration?
>>> >
>>> > icfg.setTransactionsValidator(new CustomTransactionsValidator(...));
>>> >
>>> > In that case we'll gain easy and proper solution allows to check
>>> > transaction configuration before real tx creation.
>>> >
>>> > It will be necessary to add some getters to IgniteTransactions:
>>> > - label()
>>> > - timeout()
>>> > - concurrency() (optional)
>>> > - isolation() (optional)
>>> > - txSize() (optional)
>>> >
>>> > Thoughts?
>>> >
>>> > пн, 21 мая 2018 г. в 16:31, Yakov Zhdanov <yz...@apache.org>:
>>> >
>>> > > Ok, then it probably might have been created before this PR. Anyway,
>>> I
>>> > > would not bother too much about pt 3.
>>> > >
>>> > > --Yakov
>>> > >
>>> > > 2018-05-21 16:15 GMT+03:00 Dmitry Pavlov <dp...@gmail.com>:
>>> > >
>>> > > > Hi Yakov,
>>> > > >
>>> > > > I've checked this code and IgniteCacheTestSuite6 includes
>>> TxLabelTest,
>>> > so
>>> > > > point 3 can be considered as solved.
>>> > > >
>>> > > > Sincerely,
>>> > > > Dmitriy Pavlov
>>> > > >
>>> > > > пн, 21 мая 2018 г. в 16:05, Yakov Zhdanov <yz...@apache.org>:
>>> > > >
>>> > > > > Anton, I have objections. Please do not merge unless we agree on
>>> > > details.
>>> > > > >
>>> > > > > 1. You use internal API in public event, i.e. you cannot have
>>> user
>>> > > > > accessing to IgniteInternalTx instance through TxEvent.
>>> > > > > 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.
>>> > > > > 3. TxLabelTest is not included into any suite.
>>> > > > > 4. EVT_TX_STARTED - does not seems to be a proper and descriptive
>>> > name
>>> > > > >
>>> > > > > I think that we should think about some other solution instead of
>>> > > > altering
>>> > > > > event sub-system.
>>> > > > >
>>> > > > > Also I want to ask everyone in community to request explicit
>>> approval
>>> > > > from
>>> > > > > community members before changing anything in transactional
>>> logic.
>>> > > > >
>>> > > > > Thanks!
>>> > > > >
>>> > > > > --Yakov
>>> > > > >
>>> > > >
>>> > >
>>> >
>>>
>>

Re: Ability to check and completely fill transactions on creation

Posted by Anton Vinogradov <av...@apache.org>.
One more case is to analize and log tx's creators info without tx creation
restriction.
This is very important feature on huge system trial period when you may
want to check who creates tx, tx content and configuration.
For example you may want to log stacktrace for any tx without meta or with
empty timeout.
This will allow you to find a team responsible for that and make sure that
they will fix their code.

пн, 21 мая 2018 г. в 18:14, Anton Vinogradov <av...@apache.org>:

> Dmitriy,
>
> Main idea is to restrict transaction creation in case label or timeout are
> not set.
> But there can be variations, for example label should fit some regexp or
> timeout should be between 30 and 5000 ms.
>
> That's not easy to restrict this at user code when you have dozens of
> libraries written by different people in one system using one ignite
> cluster.
> That solution based on idea of TopologyValidator when you have no chances
> to use cluster in case something wrong with nodes.
> So, any client should have no chances to create transaction not suitable
> for this ignite cluster.
>
> пн, 21 мая 2018 г. в 17:48, Dmitriy Setrakyan <ds...@apache.org>:
>
>> Anton,
>>
>> The change looks very questionable. We cannot be adding configuration
>> validators for every piece of Ignite API. What is it you are trying to
>> achieve?
>>
>> D.
>>
>> On Mon, May 21, 2018 at 7:22 AM, Anton Vinogradov <av...@apache.org> wrote:
>>
>> > Yakov, thank's for deep check.
>> >
>> > >> I think that we should think about some other solution instead of
>> > altering
>> > >> event sub-system.
>> >
>> > Thank's to your comments now I see that solution is not perfect.
>> >
>> > How about to create
>> >
>> > interface TransactionsValidator {
>> >    boolean validate(IgniteTransactions tx){
>> >       ...
>> >    }
>> > }
>> >
>> > and add it's setter to IgniteConfiguration?
>> >
>> > icfg.setTransactionsValidator(new CustomTransactionsValidator(...));
>> >
>> > In that case we'll gain easy and proper solution allows to check
>> > transaction configuration before real tx creation.
>> >
>> > It will be necessary to add some getters to IgniteTransactions:
>> > - label()
>> > - timeout()
>> > - concurrency() (optional)
>> > - isolation() (optional)
>> > - txSize() (optional)
>> >
>> > Thoughts?
>> >
>> > пн, 21 мая 2018 г. в 16:31, Yakov Zhdanov <yz...@apache.org>:
>> >
>> > > Ok, then it probably might have been created before this PR. Anyway, I
>> > > would not bother too much about pt 3.
>> > >
>> > > --Yakov
>> > >
>> > > 2018-05-21 16:15 GMT+03:00 Dmitry Pavlov <dp...@gmail.com>:
>> > >
>> > > > Hi Yakov,
>> > > >
>> > > > I've checked this code and IgniteCacheTestSuite6 includes
>> TxLabelTest,
>> > so
>> > > > point 3 can be considered as solved.
>> > > >
>> > > > Sincerely,
>> > > > Dmitriy Pavlov
>> > > >
>> > > > пн, 21 мая 2018 г. в 16:05, Yakov Zhdanov <yz...@apache.org>:
>> > > >
>> > > > > Anton, I have objections. Please do not merge unless we agree on
>> > > details.
>> > > > >
>> > > > > 1. You use internal API in public event, i.e. you cannot have user
>> > > > > accessing to IgniteInternalTx instance through TxEvent.
>> > > > > 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.
>> > > > > 3. TxLabelTest is not included into any suite.
>> > > > > 4. EVT_TX_STARTED - does not seems to be a proper and descriptive
>> > name
>> > > > >
>> > > > > I think that we should think about some other solution instead of
>> > > > altering
>> > > > > event sub-system.
>> > > > >
>> > > > > Also I want to ask everyone in community to request explicit
>> approval
>> > > > from
>> > > > > community members before changing anything in transactional logic.
>> > > > >
>> > > > > Thanks!
>> > > > >
>> > > > > --Yakov
>> > > > >
>> > > >
>> > >
>> >
>>
>

Re: Ability to check and completely fill transactions on creation

Posted by Anton Vinogradov <av...@apache.org>.
Dmitriy,

Main idea is to restrict transaction creation in case label or timeout are
not set.
But there can be variations, for example label should fit some regexp or
timeout should be between 30 and 5000 ms.

That's not easy to restrict this at user code when you have dozens of
libraries written by different people in one system using one ignite
cluster.
That solution based on idea of TopologyValidator when you have no chances
to use cluster in case something wrong with nodes.
So, any client should have no chances to create transaction not suitable
for this ignite cluster.

пн, 21 мая 2018 г. в 17:48, Dmitriy Setrakyan <ds...@apache.org>:

> Anton,
>
> The change looks very questionable. We cannot be adding configuration
> validators for every piece of Ignite API. What is it you are trying to
> achieve?
>
> D.
>
> On Mon, May 21, 2018 at 7:22 AM, Anton Vinogradov <av...@apache.org> wrote:
>
> > Yakov, thank's for deep check.
> >
> > >> I think that we should think about some other solution instead of
> > altering
> > >> event sub-system.
> >
> > Thank's to your comments now I see that solution is not perfect.
> >
> > How about to create
> >
> > interface TransactionsValidator {
> >    boolean validate(IgniteTransactions tx){
> >       ...
> >    }
> > }
> >
> > and add it's setter to IgniteConfiguration?
> >
> > icfg.setTransactionsValidator(new CustomTransactionsValidator(...));
> >
> > In that case we'll gain easy and proper solution allows to check
> > transaction configuration before real tx creation.
> >
> > It will be necessary to add some getters to IgniteTransactions:
> > - label()
> > - timeout()
> > - concurrency() (optional)
> > - isolation() (optional)
> > - txSize() (optional)
> >
> > Thoughts?
> >
> > пн, 21 мая 2018 г. в 16:31, Yakov Zhdanov <yz...@apache.org>:
> >
> > > Ok, then it probably might have been created before this PR. Anyway, I
> > > would not bother too much about pt 3.
> > >
> > > --Yakov
> > >
> > > 2018-05-21 16:15 GMT+03:00 Dmitry Pavlov <dp...@gmail.com>:
> > >
> > > > Hi Yakov,
> > > >
> > > > I've checked this code and IgniteCacheTestSuite6 includes
> TxLabelTest,
> > so
> > > > point 3 can be considered as solved.
> > > >
> > > > Sincerely,
> > > > Dmitriy Pavlov
> > > >
> > > > пн, 21 мая 2018 г. в 16:05, Yakov Zhdanov <yz...@apache.org>:
> > > >
> > > > > Anton, I have objections. Please do not merge unless we agree on
> > > details.
> > > > >
> > > > > 1. You use internal API in public event, i.e. you cannot have user
> > > > > accessing to IgniteInternalTx instance through TxEvent.
> > > > > 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.
> > > > > 3. TxLabelTest is not included into any suite.
> > > > > 4. EVT_TX_STARTED - does not seems to be a proper and descriptive
> > name
> > > > >
> > > > > I think that we should think about some other solution instead of
> > > > altering
> > > > > event sub-system.
> > > > >
> > > > > Also I want to ask everyone in community to request explicit
> approval
> > > > from
> > > > > community members before changing anything in transactional logic.
> > > > >
> > > > > Thanks!
> > > > >
> > > > > --Yakov
> > > > >
> > > >
> > >
> >
>

Re: Ability to check and completely fill transactions on creation

Posted by Dmitriy Setrakyan <ds...@apache.org>.
Anton,

The change looks very questionable. We cannot be adding configuration
validators for every piece of Ignite API. What is it you are trying to
achieve?

D.

On Mon, May 21, 2018 at 7:22 AM, Anton Vinogradov <av...@apache.org> wrote:

> Yakov, thank's for deep check.
>
> >> I think that we should think about some other solution instead of
> altering
> >> event sub-system.
>
> Thank's to your comments now I see that solution is not perfect.
>
> How about to create
>
> interface TransactionsValidator {
>    boolean validate(IgniteTransactions tx){
>       ...
>    }
> }
>
> and add it's setter to IgniteConfiguration?
>
> icfg.setTransactionsValidator(new CustomTransactionsValidator(...));
>
> In that case we'll gain easy and proper solution allows to check
> transaction configuration before real tx creation.
>
> It will be necessary to add some getters to IgniteTransactions:
> - label()
> - timeout()
> - concurrency() (optional)
> - isolation() (optional)
> - txSize() (optional)
>
> Thoughts?
>
> пн, 21 мая 2018 г. в 16:31, Yakov Zhdanov <yz...@apache.org>:
>
> > Ok, then it probably might have been created before this PR. Anyway, I
> > would not bother too much about pt 3.
> >
> > --Yakov
> >
> > 2018-05-21 16:15 GMT+03:00 Dmitry Pavlov <dp...@gmail.com>:
> >
> > > Hi Yakov,
> > >
> > > I've checked this code and IgniteCacheTestSuite6 includes TxLabelTest,
> so
> > > point 3 can be considered as solved.
> > >
> > > Sincerely,
> > > Dmitriy Pavlov
> > >
> > > пн, 21 мая 2018 г. в 16:05, Yakov Zhdanov <yz...@apache.org>:
> > >
> > > > Anton, I have objections. Please do not merge unless we agree on
> > details.
> > > >
> > > > 1. You use internal API in public event, i.e. you cannot have user
> > > > accessing to IgniteInternalTx instance through TxEvent.
> > > > 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.
> > > > 3. TxLabelTest is not included into any suite.
> > > > 4. EVT_TX_STARTED - does not seems to be a proper and descriptive
> name
> > > >
> > > > I think that we should think about some other solution instead of
> > > altering
> > > > event sub-system.
> > > >
> > > > Also I want to ask everyone in community to request explicit approval
> > > from
> > > > community members before changing anything in transactional logic.
> > > >
> > > > Thanks!
> > > >
> > > > --Yakov
> > > >
> > >
> >
>

Re: Ability to check and completely fill transactions on creation

Posted by Anton Vinogradov <av...@apache.org>.
Yakov, thank's for deep check.

>> I think that we should think about some other solution instead of
altering
>> event sub-system.

Thank's to your comments now I see that solution is not perfect.

How about to create

interface TransactionsValidator {
   boolean validate(IgniteTransactions tx){
      ...
   }
}

and add it's setter to IgniteConfiguration?

icfg.setTransactionsValidator(new CustomTransactionsValidator(...));

In that case we'll gain easy and proper solution allows to check
transaction configuration before real tx creation.

It will be necessary to add some getters to IgniteTransactions:
- label()
- timeout()
- concurrency() (optional)
- isolation() (optional)
- txSize() (optional)

Thoughts?

пн, 21 мая 2018 г. в 16:31, Yakov Zhdanov <yz...@apache.org>:

> Ok, then it probably might have been created before this PR. Anyway, I
> would not bother too much about pt 3.
>
> --Yakov
>
> 2018-05-21 16:15 GMT+03:00 Dmitry Pavlov <dp...@gmail.com>:
>
> > Hi Yakov,
> >
> > I've checked this code and IgniteCacheTestSuite6 includes TxLabelTest, so
> > point 3 can be considered as solved.
> >
> > Sincerely,
> > Dmitriy Pavlov
> >
> > пн, 21 мая 2018 г. в 16:05, Yakov Zhdanov <yz...@apache.org>:
> >
> > > Anton, I have objections. Please do not merge unless we agree on
> details.
> > >
> > > 1. You use internal API in public event, i.e. you cannot have user
> > > accessing to IgniteInternalTx instance through TxEvent.
> > > 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.
> > > 3. TxLabelTest is not included into any suite.
> > > 4. EVT_TX_STARTED - does not seems to be a proper and descriptive name
> > >
> > > I think that we should think about some other solution instead of
> > altering
> > > event sub-system.
> > >
> > > Also I want to ask everyone in community to request explicit approval
> > from
> > > community members before changing anything in transactional logic.
> > >
> > > Thanks!
> > >
> > > --Yakov
> > >
> >
>

Re: Ability to check and completely fill transactions on creation

Posted by Yakov Zhdanov <yz...@apache.org>.
Ok, then it probably might have been created before this PR. Anyway, I
would not bother too much about pt 3.

--Yakov

2018-05-21 16:15 GMT+03:00 Dmitry Pavlov <dp...@gmail.com>:

> Hi Yakov,
>
> I've checked this code and IgniteCacheTestSuite6 includes TxLabelTest, so
> point 3 can be considered as solved.
>
> Sincerely,
> Dmitriy Pavlov
>
> пн, 21 мая 2018 г. в 16:05, Yakov Zhdanov <yz...@apache.org>:
>
> > Anton, I have objections. Please do not merge unless we agree on details.
> >
> > 1. You use internal API in public event, i.e. you cannot have user
> > accessing to IgniteInternalTx instance through TxEvent.
> > 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.
> > 3. TxLabelTest is not included into any suite.
> > 4. EVT_TX_STARTED - does not seems to be a proper and descriptive name
> >
> > I think that we should think about some other solution instead of
> altering
> > event sub-system.
> >
> > Also I want to ask everyone in community to request explicit approval
> from
> > community members before changing anything in transactional logic.
> >
> > Thanks!
> >
> > --Yakov
> >
>

Re: Ability to check and completely fill transactions on creation

Posted by Dmitry Pavlov <dp...@gmail.com>.
Hi Yakov,

I've checked this code and IgniteCacheTestSuite6 includes TxLabelTest, so
point 3 can be considered as solved.

Sincerely,
Dmitriy Pavlov

пн, 21 мая 2018 г. в 16:05, Yakov Zhdanov <yz...@apache.org>:

> Anton, I have objections. Please do not merge unless we agree on details.
>
> 1. You use internal API in public event, i.e. you cannot have user
> accessing to IgniteInternalTx instance through TxEvent.
> 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.
> 3. TxLabelTest is not included into any suite.
> 4. EVT_TX_STARTED - does not seems to be a proper and descriptive name
>
> I think that we should think about some other solution instead of altering
> event sub-system.
>
> Also I want to ask everyone in community to request explicit approval from
> community members before changing anything in transactional logic.
>
> Thanks!
>
> --Yakov
>

Re: Ability to check and completely fill transactions on creation

Posted by Yakov Zhdanov <yz...@apache.org>.
Anton, I have objections. Please do not merge unless we agree on details.

1. You use internal API in public event, i.e. you cannot have user
accessing to IgniteInternalTx instance through TxEvent.
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.
3. TxLabelTest is not included into any suite.
4. EVT_TX_STARTED - does not seems to be a proper and descriptive name

I think that we should think about some other solution instead of altering
event sub-system.

Also I want to ask everyone in community to request explicit approval from
community members before changing anything in transactional logic.

Thanks!

--Yakov