Val, Alexei - no objections from my side, thanks for the explanation. On Tue, Nov 30, 2021 at 1:18 PM Alexei Scherbakov < alexey.scherbak...@gmail.com> wrote:
> Pavel, I agree with Val to avoid overloading due to a loss of API > transparency. > > Val, moving the tx argument at the first position seems good to me. > > пн, 29 нояб. 2021 г. в 22:03, Valentin Kulichenko < > valentin.kuliche...@gmail.com>: > > > Alexei, > > > > One more comment: I actually think that the transaction should be the > first > > argument, not the last. This way it's easier to keep the API consistent. > > For example, if a method uses varargs as one of the parameters, you won't > > be able to put the tx parameter at the end. There might be other cases as > > well. What do you think? > > > > -Val > > > > On Mon, Nov 29, 2021 at 10:59 AM Valentin Kulichenko < > > valentin.kuliche...@gmail.com> wrote: > > > > > I like Alexei's suggestion. This seems to be the most transparent and > > > explicit approach. Basically, this ensures that the user is always > aware > > of > > > whether an operation is enlisted in a transaction or not. Any other > > option > > > is either error-prone, or introduces unnecessary counter-intuitive > > > limitations. > > > > > > I don't think we should keep overloads without the tx parameter, > because > > > that will pretty much eliminate the value of this change. One thing we > > can > > > do to address this is to have separate "non-tx" views, which can only > be > > > used to execute implicit transactions. But I would look at this after > we > > > more or less stabilize the primary API. > > > > > > -Val > > > > > > On Mon, Nov 29, 2021 at 5:03 AM Pavel Tupitsyn <ptupit...@apache.org> > > > wrote: > > > > > >> Alexei, > > >> > > >> Are we going to offer an overload without tx parameter? > > >> > > >> getAsync(K key); > > >> getAsync(K key, Transaction tx); > > >> > > >> On Mon, Nov 29, 2021 at 3:43 PM Alexei Scherbakov < > > >> alexey.scherbak...@gmail.com> wrote: > > >> > > >> > Pavel, > > >> > > > >> > The problem with a current approach to me is the possibility of > > >> forgetting > > >> > to enlist a table into a transaction, because it is not enforced. > > >> > Having the explicit argument for this purpose seems less error-prone > > to > > >> me. > > >> > > > >> > пн, 29 нояб. 2021 г. в 15:13, Pavel Tupitsyn <ptupit...@apache.org > >: > > >> > > > >> > > Taras, yes, yours is the actual syntax in main branch right now, > > >> > > I've skipped the tx argument in my code accidentally. > > >> > > > > >> > > On Mon, Nov 29, 2021 at 3:03 PM Taras Ledkov < > tled...@gridgain.com> > > >> > wrote: > > >> > > > > >> > > > Hi colleagues, > > >> > > > > > >> > > > 2Pavel: > > >> > > > > RecordView<Tuple> txView = view.withTransaction(); > > >> > > > Can we use the syntax (see below) to attach the table / > operation > > to > > >> > the > > >> > > > started transaction? > > >> > > > RecordView<Tuple2> txPersonView = > > >> > > > person.recordView().withTransaction(txView.transaction()); > > >> > > > > > >> > > > > > >> > > > On Mon, Nov 29, 2021 at 1:34 PM Pavel Tupitsyn < > > >> ptupit...@apache.org> > > >> > > > wrote: > > >> > > > > > >> > > > > Alexei, > > >> > > > > > > >> > > > > I agree that runInTransaction is confusing and error-prone. > > >> > > > > > > >> > > > > But we already have view.withTransaction(), which seems to be > > the > > >> > most > > >> > > > > boilerplate-free approach. > > >> > > > > The example above will look like this: > > >> > > > > > > >> > > > > public void testMixedPutGet() throws TransactionException { > > >> > > > > RecordView<Tuple> view = accounts.recordView(); > > >> > > > > > > >> > > > > view.upsert(makeValue(1, BALANCE_1)); > > >> > > > > > > >> > > > > RecordView<Tuple> txView = view.withTransaction(); > > >> > > > > > > >> > > > > txView.getAsync(makeKey(1)).thenCompose(r -> > > >> > > > > txView.upsertAsync(makeValue(1, r.doubleValue("balance") + > > DELTA), > > >> > > > > tx)).thenCompose(txView.transaction().commitAsync()).join(); > > >> > > > > > > >> > > > > assertEquals(BALANCE_1 + DELTA, > > >> > > > > view.get(makeKey(1)).doubleValue("balance")); > > >> > > > > } > > >> > > > > > > >> > > > > Is there any problem with this? > > >> > > > > > > >> > > > > On Mon, Nov 29, 2021 at 10:45 AM Alexei Scherbakov < > > >> > > > > alexey.scherbak...@gmail.com> wrote: > > >> > > > > > > >> > > > > > Folks, > > >> > > > > > > > >> > > > > > Recently I've pushed transactions support phase 1 for Ignite > > 3, > > >> see > > >> > > > [1]. > > >> > > > > > Feel free to give feedback. > > >> > > > > > Current implementation attempts to automatically enlist a > > table > > >> > into > > >> > > > > > transaction if it's started using [2] or [3] by using thread > > >> local > > >> > > > > context, > > >> > > > > > similar to Ignite 2 approach, to reduce the amount of > > >> boilerplate > > >> > > code. > > >> > > > > > But it turns out such an approach still has unacceptable > > >> drawbacks > > >> > > > from a > > >> > > > > > user experience point of view. > > >> > > > > > > > >> > > > > > Consider the example [4]: > > >> > > > > > > > >> > > > > > public void testMixedPutGet() throws TransactionException { > > >> > > > > > accounts.recordView().upsert(makeValue(1, > BALANCE_1)); > > >> > > > > > > > >> > > > > > igniteTransactions.runInTransaction(tx -> { > > >> > > > > > var txAcc = > > >> accounts.recordView().withTransaction(tx); > > >> > > > > > > > >> > > > > > txAcc.getAsync(makeKey(1)).thenCompose(r -> > > >> > > > > > txAcc.upsertAsync(makeValue(1, r.doubleValue("balance") + > > >> > > > > DELTA))).join(); > > >> > > > > > }); > > >> > > > > > > > >> > > > > > assertEquals(BALANCE_1 + DELTA, > > >> > > > > > > accounts.recordView().get(makeKey(1)).doubleValue("balance")); > > >> > > > > > } > > >> > > > > > > > >> > > > > > Here we *have to* to manually enlist a table if it's used in > > >> async > > >> > > > chain > > >> > > > > > call, because the caller thread will be different and the > > >> chained > > >> > > > > operation > > >> > > > > > will be executed in separate tx. > > >> > > > > > This works similarly in Ignite 2 and is very confusing. > > >> > > > > > > > >> > > > > > To avoid this, I propose to add an explicit Transaction > > >> argument to > > >> > > > each > > >> > > > > > table API method. Null value means to start the implicit > > >> > transaction > > >> > > > > > (autocommit mode). For example: > > >> > > > > > > > >> > > > > > /** > > >> > > > > > * Asynchronously inserts a record into the table if it > > >> doesn't > > >> > > > exist > > >> > > > > > or replaces the existed one. > > >> > > > > > * > > >> > > > > > * @param rec A record to insert into the table. The > > record > > >> > > cannot > > >> > > > be > > >> > > > > > {@code null}. > > >> > > > > > * @param tx The transaction or {@code null} to auto > > commit. > > >> > > > > > * @return Future representing pending completion of the > > >> > > operation. > > >> > > > > > */ > > >> > > > > > @NotNull CompletableFuture<Void> upsertAsync(@NotNull R > > rec, > > >> > > > > @Nullable > > >> > > > > > Transaction tx); > > >> > > > > > > > >> > > > > > The example [4] turns to > > >> > > > > > > > >> > > > > > public void testMixedPutGet() throws TransactionException { > > >> > > > > > RecordView<Tuple> view = accounts.recordView(); > > >> > > > > > > > >> > > > > > view.upsert(makeValue(1, BALANCE_1)); > > >> > > > > > > > >> > > > > > igniteTransactions.runInTransaction(tx -> { > > >> > > > > > view.getAsync(makeKey(1), tx).thenCompose(r -> > > >> > > > > > view.upsertAsync(makeValue(1, r.doubleValue("balance") + > > DELTA), > > >> > > > > > tx)).join(); > > >> > > > > > }); > > >> > > > > > > > >> > > > > > assertEquals(BALANCE_1 + DELTA, > > >> > > > > > view.get(makeKey(1)).doubleValue("balance")); > > >> > > > > > } > > >> > > > > > > > >> > > > > > Share your thoughts. > > >> > > > > > > > >> > > > > > [1] https://issues.apache.org/jira/browse/IGNITE-15085 > > >> > > > > > [2] > > >> > > > > > > > >> > > > > > > >> > > > > > >> > > > > >> > > > >> > > > org.apache.ignite.tx.IgniteTransactions#runInTransaction(java.util.function.Consumer<org.apache.ignite.tx.Transaction>) > > >> > > > > > [3] > > >> > > > > > > > >> > > > > > > >> > > > > > >> > > > > >> > > > >> > > > org.apache.ignite.tx.IgniteTransactions#runInTransaction(java.util.function.Function<org.apache.ignite.tx.Transaction,T>) > > >> > > > > > [4] > > >> org.apache.ignite.internal.table.TxAbstractTest#testMixedPutGet > > >> > > > > > > > >> > > > > > ср, 14 июл. 2021 г. в 14:12, Alexei Scherbakov < > > >> > > > > > alexey.scherbak...@gmail.com > > >> > > > > > >: > > >> > > > > > > > >> > > > > > > Andrey, > > >> > > > > > > > > >> > > > > > > 1) "As a user, I'd expect runInTransaction(closure) will > > >> create > > >> > Tx > > >> > > > for > > >> > > > > > me, > > >> > > > > > > commit Tx after a successful closure call, and rollback Tx > > in > > >> > case > > >> > > of > > >> > > > > > > error." > > >> > > > > > > - I'm ok with this behavior, and will alter javadoc. > > >> > > > > > > > > >> > > > > > > 2) "Transaction tx = beginTx()" - there is no such method > > >> > "beginTx" > > >> > > > in > > >> > > > > > the > > >> > > > > > > proposed API, and I'm not intending to add it. > > >> > > > > > > For the synchronous case I suggest to use > > "runInTransaction", > > >> > which > > >> > > > > > > eliminates the need in AutoClosable. > > >> > > > > > > > > >> > > > > > > > > >> > > > > > > > > >> > > > > > > ср, 14 июл. 2021 г. в 13:21, Ivan Daschinsky < > > >> > ivanda...@gmail.com > > >> > > >: > > >> > > > > > > > > >> > > > > > >> > yes, it is stated in the javadoc in the PR. > > >> > > > > > >> Ah, I see. > > >> > > > > > >> > > >> > > > > > >> ср, 14 июл. 2021 г. в 12:16, Alexei Scherbakov < > > >> > > > > > >> alexey.scherbak...@gmail.com > > >> > > > > > >> >: > > >> > > > > > >> > > >> > > > > > >> > Ivan, > > >> > > > > > >> > > > >> > > > > > >> > And what if I have already committed transaction? Is it > > >> safe > > >> > > > > rollback > > >> > > > > > >> > already committed transaction? Rollback will silently > > >> return > > >> > and > > >> > > > do > > >> > > > > > >> > nothing? - yes, it is stated in the javadoc in the PR. > > >> > > > > > >> > > > >> > > > > > >> > Andrey, > > >> > > > > > >> > > > >> > > > > > >> > Then using "runInTransaction", lack of commit will > cause > > a > > >> > > > > transaction > > >> > > > > > >> to > > >> > > > > > >> > rollback automatically. > > >> > > > > > >> > > > >> > > > > > >> > There is no need for a "close" method, it just adds > > >> confusion. > > >> > > > > > >> > > > >> > > > > > >> > > > >> > > > > > >> > ср, 14 июл. 2021 г. в 11:37, Andrey Mashenkov < > > >> > > > > > >> andrey.mashen...@gmail.com > > >> > > > > > >> > >: > > >> > > > > > >> > > > >> > > > > > >> > > Agree with Ivan. > > >> > > > > > >> > > > > >> > > > > > >> > > Method runInTransaction() should try to finish the > > >> > transaction > > >> > > > if > > >> > > > > > the > > >> > > > > > >> > user > > >> > > > > > >> > > forgot to commit one. > > >> > > > > > >> > > I guess it might be a common mistake among new users. > > >> > > > > > >> > > > > >> > > > > > >> > > Also, I suggest to extent all table projections for > > >> better > > >> > UX. > > >> > > > > > >> > > Let's allow > > >> > > > > > >> > > table.kvView().withTx(tx) > > >> > > > > > >> > > to user may cache kvVew instance and do > > >> > > > > > >> > > kvView.withTx(tx) > > >> > > > > > >> > > rather than > > >> > > > > > >> > > table.withTx(tx).kvVew() > > >> > > > > > >> > > > > >> > > > > > >> > > > > >> > > > > > >> > > > > >> > > > > > >> > > On Wed, Jul 14, 2021 at 10:13 AM Ivan Daschinsky < > > >> > > > > > ivanda...@gmail.com > > >> > > > > > >> > > > >> > > > > > >> > > wrote: > > >> > > > > > >> > > > > >> > > > > > >> > > > Alexey, and is there any analogue to close() of > > >> > transaction? > > >> > > > > When > > >> > > > > > >> you > > >> > > > > > >> > > start > > >> > > > > > >> > > > transaction, you should somehow to close it, if you > > >> don't > > >> > > > catch > > >> > > > > > >> > exception > > >> > > > > > >> > > > or forget to commit. > > >> > > > > > >> > > > > > >> > > > > > >> > > > I suggest to add method closeAsync() to > Transaction, > > so > > >> > user > > >> > > > can > > >> > > > > > >> call > > >> > > > > > >> > it > > >> > > > > > >> > > in > > >> > > > > > >> > > > handle or whenComplete, i.e. > > >> > > > > > >> > > > > > >> > > > > > >> > > > So code will looks like > > >> > > > > > >> > > > > > >> > > > > > >> > > > CacheApi cache = CacheApi.getCache("testCache"); > > >> > > > > > >> > > > > > >> > > > > > >> > > > Transactions > > >> > > > > > >> > > > .beginTransaction() > > >> > > > > > >> > > > .thenCompose(tx -> { > > >> > > > > > >> > > > CacheApi txCache = cache.withTx(tx); > > >> > > > > > >> > > > CompletableFuture<Void> result = > > >> > > > txCache.getAsync("key") > > >> > > > > > >> > > > .thenCompose(val -> { > > >> > > > > > >> > > > if (val == "test") { > > >> > > > > > >> > > > return txCache.putAsync("key", > > >> > "test1"); > > >> > > > > > >> > > > } > > >> > > > > > >> > > > else > > >> > > > > > >> > > > return > > >> > > > > > CompletableFuture.completedFuture(null); > > >> > > > > > >> > > > }) > > >> > > > > > >> > > > .thenCompose(v -> tx.commitAsync()) > > >> > > > > > >> > > > .handle((v, ex) -> null); > > >> > > > > > >> > > > return result.thenCompose(v -> > > >> tx.closeAsync()); > > >> > > > > > >> > > > }); > > >> > > > > > >> > > > > > >> > > > > > >> > > > I also suggests to add method something like this > > >> > > > > > >> > > > > > >> > > > > > >> > > > static CompletableFuture<Void> > > >> > > inTxAsync(Function<Transaction, > > >> > > > > > >> > > > CompletableFuture<Void>> action) { > > >> > > > > > >> > > > return Transactions > > >> > > > > > >> > > > .beginTransaction() > > >> > > > > > >> > > > .thenCompose(tx -> { > > >> > > > > > >> > > > CompletableFuture<Object> result = > > >> > > > action.apply(tx) > > >> > > > > > >> > > > .handle((v, ex) -> null); > > >> > > > > > >> > > > return result.thenCompose(v -> > > >> > tx.closeAsync()); > > >> > > > > > >> > > > }); > > >> > > > > > >> > > > } > > >> > > > > > >> > > > > > >> > > > > > >> > > > Async api is not very readable, but this method can > > >> help > > >> > > user > > >> > > > > > write > > >> > > > > > >> > code, > > >> > > > > > >> > > > this is rewritten first example: > > >> > > > > > >> > > > > > >> > > > > > >> > > > Transactions.inTxAsync(tx -> { > > >> > > > > > >> > > > CacheApi txCache = cache.withTx(tx); > > >> > > > > > >> > > > return txCache.getAsync("key") > > >> > > > > > >> > > > .thenCompose(val -> { > > >> > > > > > >> > > > if (val == "test") { > > >> > > > > > >> > > > return txCache.putAsync("key", > > >> "test1"); > > >> > > > > > >> > > > } > > >> > > > > > >> > > > else > > >> > > > > > >> > > > return > > >> > > > CompletableFuture.completedFuture(null); > > >> > > > > > >> > > > }) > > >> > > > > > >> > > > .thenCompose(v -> tx.commitAsync()); > > >> > > > > > >> > > > }); > > >> > > > > > >> > > > > > >> > > > > > >> > > > ср, 14 июл. 2021 г. в 10:03, Alexei Scherbakov < > > >> > > > > > >> > > > alexey.scherbak...@gmail.com > > >> > > > > > >> > > > >: > > >> > > > > > >> > > > > > >> > > > > > >> > > > > Andrey, > > >> > > > > > >> > > > > > > >> > > > > > >> > > > > I suggest you look at the PR [1], if you haven't. > > >> > > > > > >> > > > > > > >> > > > > > >> > > > > A transaction [2] > > >> > > > > > >> > > > > Transactions facade [3] > > >> > > > > > >> > > > > Examples [4] > > >> > > > > > >> > > > > > > >> > > > > > >> > > > > [1] > > >> https://github.com/apache/ignite-3/pull/214/files > > >> > > > > > >> > > > > [2] > > >> > > > > > >> > > > > > > >> > > > > > >> > > > > > > >> > > > > > >> > > > > > >> > > > > > >> > > > > >> > > > > > >> > > > >> > > > > > >> > > >> > > > > > > > >> > > > > > > >> > > > > > >> > > > > >> > > > >> > > > https://github.com/apache/ignite-3/blob/d2122ce8c15de020e121f53509bd5a097aac9cf2/modules/api/src/main/java/org/apache/ignite/tx/Transaction.java > > >> > > > > > >> > > > > [3] > > >> > > > > > >> > > > > > > >> > > > > > >> > > > > > > >> > > > > > >> > > > > > >> > > > > > >> > > > > >> > > > > > >> > > > >> > > > > > >> > > >> > > > > > > > >> > > > > > > >> > > > > > >> > > > > >> > > > >> > > > https://github.com/apache/ignite-3/blob/d2122ce8c15de020e121f53509bd5a097aac9cf2/modules/api/src/main/java/org/apache/ignite/tx/IgniteTransactions.java > > >> > > > > > >> > > > > [4] > > >> > > > > > >> > > > > > > >> > > > > > >> > > > > > > >> > > > > > >> > > > > > >> > > > > > >> > > > > >> > > > > > >> > > > >> > > > > > >> > > >> > > > > > > > >> > > > > > > >> > > > > > >> > > > > >> > > > >> > > > https://github.com/apache/ignite-3/blob/d2122ce8c15de020e121f53509bd5a097aac9cf2/modules/table/src/test/java/org/apache/ignite/internal/table/TxTest.java > > >> > > > > > >> > > > > > > >> > > > > > >> > > > > > > >> > > > > > >> > > > > вт, 13 июл. 2021 г. в 19:41, Andrey Gura < > > >> > > ag...@apache.org > > >> > > > >: > > >> > > > > > >> > > > > > > >> > > > > > >> > > > > > Alexey, > > >> > > > > > >> > > > > > > > >> > > > > > >> > > > > > could you please describe Transaction > interface? > > >> > > > > > >> > > > > > > > >> > > > > > >> > > > > > Also it would be great to have a couple > examples > > of > > >> > > using > > >> > > > > the > > >> > > > > > >> > > proposed > > >> > > > > > >> > > > > API. > > >> > > > > > >> > > > > > > > >> > > > > > >> > > > > > On Tue, Jul 13, 2021 at 4:43 PM Alexei > Scherbakov > > >> > > > > > >> > > > > > <alexey.scherbak...@gmail.com> wrote: > > >> > > > > > >> > > > > > > > > >> > > > > > >> > > > > > > Folks, > > >> > > > > > >> > > > > > > > > >> > > > > > >> > > > > > > I've prepared a PR implementing my vision of > > >> public > > >> > > > > > >> transactions > > >> > > > > > >> > > API. > > >> > > > > > >> > > > > > > > > >> > > > > > >> > > > > > > API is very simple and similar to Ignite 2, > but > > >> has > > >> > > some > > >> > > > > > >> > > differences. > > >> > > > > > >> > > > > > > > > >> > > > > > >> > > > > > > More details can be found here [1] > > >> > > > > > >> > > > > > > > > >> > > > > > >> > > > > > > Share your thoughts. > > >> > > > > > >> > > > > > > > > >> > > > > > >> > > > > > > [1] > > >> > > https://issues.apache.org/jira/browse/IGNITE-15086 > > >> > > > > > >> > > > > > > > > >> > > > > > >> > > > > > > -- > > >> > > > > > >> > > > > > > > > >> > > > > > >> > > > > > > Best regards, > > >> > > > > > >> > > > > > > Alexei Scherbakov > > >> > > > > > >> > > > > > > > >> > > > > > >> > > > > > > >> > > > > > >> > > > > > > >> > > > > > >> > > > > -- > > >> > > > > > >> > > > > > > >> > > > > > >> > > > > Best regards, > > >> > > > > > >> > > > > Alexei Scherbakov > > >> > > > > > >> > > > > > > >> > > > > > >> > > > > > >> > > > > > >> > > > > > >> > > > > > >> > > > -- > > >> > > > > > >> > > > Sincerely yours, Ivan Daschinskiy > > >> > > > > > >> > > > > > >> > > > > > >> > > > > >> > > > > > >> > > > > >> > > > > > >> > > -- > > >> > > > > > >> > > Best regards, > > >> > > > > > >> > > Andrey V. Mashenkov > > >> > > > > > >> > > > > >> > > > > > >> > > > >> > > > > > >> > > > >> > > > > > >> > -- > > >> > > > > > >> > > > >> > > > > > >> > Best regards, > > >> > > > > > >> > Alexei Scherbakov > > >> > > > > > >> > > > >> > > > > > >> > > >> > > > > > >> > > >> > > > > > >> -- > > >> > > > > > >> Sincerely yours, Ivan Daschinskiy > > >> > > > > > >> > > >> > > > > > > > > >> > > > > > > > > >> > > > > > > -- > > >> > > > > > > > > >> > > > > > > Best regards, > > >> > > > > > > Alexei Scherbakov > > >> > > > > > > > > >> > > > > > > > >> > > > > > > > >> > > > > > -- > > >> > > > > > > > >> > > > > > Best regards, > > >> > > > > > Alexei Scherbakov > > >> > > > > > > > >> > > > > > > >> > > > > > >> > > > > >> > > > >> > > > >> > -- > > >> > > > >> > Best regards, > > >> > Alexei Scherbakov > > >> > > > >> > > > > > > > > -- > > Best regards, > Alexei Scherbakov >