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 >