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