From dev-return-35143-archive-asf-public=cust-asf.ponee.io@ignite.apache.org Thu May 31 18:10:28 2018 Return-Path: X-Original-To: archive-asf-public@cust-asf.ponee.io Delivered-To: archive-asf-public@cust-asf.ponee.io Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by mx-eu-01.ponee.io (Postfix) with SMTP id 2C4A1180632 for ; Thu, 31 May 2018 18:10:28 +0200 (CEST) Received: (qmail 72265 invoked by uid 500); 31 May 2018 16:10:27 -0000 Mailing-List: contact dev-help@ignite.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@ignite.apache.org Delivered-To: mailing list dev@ignite.apache.org Received: (qmail 72254 invoked by uid 99); 31 May 2018 16:10:27 -0000 Received: from mail-relay.apache.org (HELO mailrelay2-lw-us.apache.org) (207.244.88.137) by apache.org (qpsmtpd/0.29) with ESMTP; Thu, 31 May 2018 16:10:27 +0000 Received: from mail-it0-f43.google.com (mail-it0-f43.google.com [209.85.214.43]) by mailrelay2-lw-us.apache.org (ASF Mail Server at mailrelay2-lw-us.apache.org) with ESMTPSA id 4DEC729B5 for ; Thu, 31 May 2018 16:10:26 +0000 (UTC) Received: by mail-it0-f43.google.com with SMTP id n64-v6so28280140itb.3 for ; Thu, 31 May 2018 09:10:26 -0700 (PDT) X-Gm-Message-State: APt69E3cbSHF5AEffn4+6AeIbRxh4C62li5pojLJM47B4fFAyF9mYq/8 2lemNXoietNrABxm6s0UWk/BUr0TmiFJDwlH0cg= X-Google-Smtp-Source: ADUXVKJNnswgzz0+Z3Y3iRlmreApPnqW9X7yP1dHjmiI9eGuiX34p7b5Rveijq7lPhLwjZEiEYqS+dSL3gpBaGeE+6Y= X-Received: by 2002:a24:c489:: with SMTP id v131-v6mr639589itf.102.1527783025671; Thu, 31 May 2018 09:10:25 -0700 (PDT) MIME-Version: 1.0 References: In-Reply-To: From: Anton Vinogradov Date: Thu, 31 May 2018 19:10:16 +0300 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: Ability to check and completely fill transactions on creation To: dev@ignite.apache.org Content-Type: multipart/alternative; boundary="0000000000008a0b01056d82b41b" --0000000000008a0b01056d82b41b Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable 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 =3D { 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 =D0=B2=D1=82, 29 =D0=BC=D0=B0=D1=8F 2018 =D0=B3. =D0=B2 20:41, Dmitriy Setr= akyan : > Anton, > > We cannot have TransactionStartedEvent without having events for all othe= r > 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, whi= ch > 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 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 dou= bt > > if > > >> it can be fully supported in the pattern you use in TxLabelTest. Aft= er > > >> looking at the mentioned test user may think that throwing runtime > error > > >> when notified on new node join may prohibit new node joining which i= s > > 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 =3D evt.tx(); > > > > if (tx.label() =3D=3D null) > > tx.tx().rollback(); > > ... > > and > > ... > > // Timeout > > Transaction tx =3D 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? > > > > =D1=87=D1=82, 24 =D0=BC=D0=B0=D1=8F 2018 =D0=B3. =D0=B2 16:32, Dmitriy = Setrakyan : > > > > > 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 t= he > > > 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 > wrote: > > > > > > > Dmitriy, > > > > > > > > Yakov is against the solution based on event sub-system > > > > >> I think that we should think about some other solution instead o= f > > > > 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? > > > > > > > > > > > > =D1=87=D1=82, 24 =D0=BC=D0=B0=D1=8F 2018 =D0=B3. =D0=B2 8:58, Dmitr= iy Setrakyan >: > > > > > > > > > On Wed, May 23, 2018 at 4:08 AM, Anton Vinogradov > > > 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 mor= e > > > > cleanly, > > > > > incorporating the feedback from Yakov. > > > > > > > > > > > > > > > --0000000000008a0b01056d82b41b--