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
