Change sync implementation on async in retrying by ArtDu · Pull Request #333 · tarantool/cartridge-java

ArtDu · 2022-12-26T14:59:54Z

We had synchronous get and sleep, now we use timeoutScheduler for these purposes. The retry is invoked by recursive method calls in the asynchronous calls section.

I haven't forgotten about:

Related issues:
Closes #299

src/main/java/io/tarantool/driver/api/retry/RequestRetryPolicy.java

bitgorbovsky

Generally it looks good to me

src/main/java/io/tarantool/driver/api/retry/TarantoolRequestRetryPolicies.java

src/main/java/io/tarantool/driver/api/retry/RequestRetryPolicy.java

akudiyar · 2022-12-29T11:28:27Z

src/main/java/io/tarantool/driver/api/retry/RequestRetryPolicy.java

 */
 public interface RequestRetryPolicy {
+
+    ScheduledExecutorService timeoutScheduler =


This field will be static and public, I believe we are not willing to do that

There is a problem that we need this ScheduledExecutorService timeoutScheduler in the interface to use solutions like this one:

RequestRetryPolicy policy = throwable -> true;

src/main/java/io/tarantool/driver/api/retry/RequestRetryPolicy.java

akudiyar

Not a full review (we need to fix at least one place first because the second is almost the same). I feel that the new code will be much easier to understand if it will have any comments regarding the logic. Currently, it is not clear what potential failure cases we are covering by what statement, which adds an additional future handler. Also, I have some doubts that this code will work as intended because we are not waiting anywhere on the modified futures after adding a handler to it.

CHANGELOG.md

akudiyar · 2022-12-29T23:14:22Z

src/main/java/io/tarantool/driver/api/retry/DefaultTimeoutScheduler.java

+/**
+ * @author Artyom Dubinin
+ */
+final class DefaultTimeoutScheduler {


I believe we don't need a separate class-container for that. We already have TarantoolRequestRetryPolicies where we can put and access this private static variable.

Moved to TarantoolRequestRetryPolicies

src/main/java/io/tarantool/driver/api/retry/RequestRetryPolicy.java

akudiyar · 2022-12-29T23:21:13Z

src/main/java/io/tarantool/driver/api/retry/RequestRetryPolicy.java

-            } while (this.canRetryRequest(ex));
-            throw new CompletionException(ex);
-        }, executor);
+    default <T> CompletableFuture<T> failAfterRequestTimeout(CompletableFuture<T> resultFuture) {


Same for this method - it may be an internal method of the new Runnable class

Created issue #341 and PR #340 with concept of it

akudiyar · 2022-12-29T23:25:18Z

src/main/java/io/tarantool/driver/api/retry/RequestRetryPolicy.java

+                return null;
+            }
+
+            while (ex instanceof ExecutionException || ex instanceof CompletionException) {


nit: This loop may be a part of some separate error handler. Not neccesasry now btw

akudiyar · 2022-12-29T23:26:44Z

src/main/java/io/tarantool/driver/api/retry/RequestRetryPolicy.java

+        CompletableFuture<T> operationFuture = operation.get();
+        CompletableFuture<T> requestTimeoutFuture = failAfterRequestTimeout(resultFuture);
+
+        operationFuture.acceptEither(requestTimeoutFuture, resultFuture::complete).exceptionally(ex -> {


What will happen when operationFuture completes normally?

ALso I don't see who is waiting on the result future produced from acceptEither. Without that the desired effect will not be achieved

What will happen when operationFuture completes normally?

resultFuture::complete

ALso I don't see who is waiting on the result future produced from acceptEither. Without that the desired effect will not be achieved

We don't need to wait this future, with waiting it will be the same synchronous stuff

akudiyar · 2022-12-29T23:29:50Z

src/main/java/io/tarantool/driver/api/retry/RequestRetryPolicy.java

+                resultFuture.completeExceptionally(ex);
+            }
+            return null;
+        }).exceptionally(ex -> {


From where comes the exception that is handled here? What is the case for it?

If previous exceptionally completed with exception like problem with incorrect user canRetryRequest, we'd go there

akudiyar · 2022-12-29T23:31:24Z

src/main/java/io/tarantool/driver/api/retry/RequestRetryPolicy.java

+            final TimeoutException ex = new TimeoutException("Request timeout after " + requestTimeout);
+            return future.completeExceptionally(ex);
+        }, requestTimeout, TimeUnit.MILLISECONDS);
+        resultFuture.whenComplete((res, ex) -> scheduledFuture.cancel(false));


I believe it will not work this way. whenComplete returns a new future on which we need to call join or get to invoke the effect. It doesn't change resultFuture itself.

I believe it will not work this way. whenComplete returns a new future on which we need to call join or get to invoke the effect. It doesn't change resultFuture itself.

We don't give a care what whenComplete return us, we should only know that resultFuture is completed. And with this knowledge we just cancel scheduledFuture cause it isn't needed already

akudiyar · 2022-12-29T23:31:35Z

src/main/java/io/tarantool/driver/api/retry/RequestRetryPolicy.java

+                ScheduledFuture<?> delayFuture = getTimeoutScheduler().schedule(() -> {
+                    runAsyncOperation(operation, resultFuture, lastExceptionWrapper);
+                }, getDelay(), TimeUnit.MILLISECONDS);
+                resultFuture.whenComplete((r, e) -> delayFuture.cancel(false));


I believe it will not work this way. whenComplete returns a new future on which we need to call join or get to invoke the effect. It doesn't change resultFuture itself.

We don't want to change resultFuture here. It's just cancel already started delayFuture, it's like optimization

@akudiyar

I've been thinking about it and here's what:

Returns a new CompletionStage with the same result or exception as this stage, that executes the given action when this stage completes.

WhenComplete returns the same result as resultFuture. We don't need to wait this whenComplete future.

akudiyar · 2023-01-09T13:31:33Z

src/test/java/io/tarantool/driver/integration/RetryingTarantoolTupleClientIT.java

    }
+
+    @Test
+    void testAsyncPerformance() {


We need to make a sync + async parallel test with the use of different threads

Added 2155f78

akudiyar

I approve this as we discussed all the details verbally, with two conditions:

CHANGELOG needs to be fixed
#341 needs to be implemented right after that PR, because these two new methods in API are a bad change, the method contents are too complex for a new reader and the unit tests for this logic are lacking.

akudiyar · 2023-01-11T17:31:59Z

CHANGELOG.md

 ### Features
 - Add `fields` option to ProxySpace for controlling the result tuple fields ([#236](https://github.com/tarantool/cartridge-java/pull/236))
 - Parse metadata from crud response ([#272](https://github.com/tarantool/cartridge-java/pull/272))
+- Do not use sync calls instead of async in retrying ([#299](https://github.com/tarantool/cartridge-java/pull/299))


This needs to be moved to unreleased

Closes #299

ArtDu force-pushed the retrying branch 2 times, most recently from d534cf5 to 3b9d4ee Compare December 27, 2022 19:33

akudiyar reviewed Dec 28, 2022

View reviewed changes

src/main/java/io/tarantool/driver/api/retry/RequestRetryPolicy.java Outdated Show resolved Hide resolved

ArtDu force-pushed the retrying branch 3 times, most recently from a89b1c5 to 759c9d2 Compare December 28, 2022 13:52

ArtDu changed the title ~~WIP: Add async call in retrying client~~ Change sync realisation on async in retrying Dec 28, 2022

ArtDu force-pushed the retrying branch 4 times, most recently from 321d835 to 774cd55 Compare December 28, 2022 14:07

ArtDu marked this pull request as ready for review December 28, 2022 14:07

ArtDu requested review from akudiyar and bitgorbovsky December 28, 2022 14:07

ArtDu force-pushed the retrying branch 2 times, most recently from f01b530 to aa5e6bf Compare December 28, 2022 14:35

bitgorbovsky previously approved these changes Dec 28, 2022

View reviewed changes

ArtDu dismissed bitgorbovsky’s stale review via 8d2dd73 December 29, 2022 09:21

ArtDu force-pushed the retrying branch from 8d2dd73 to b188ef3 Compare December 29, 2022 09:41

ArtDu requested a review from bitgorbovsky December 29, 2022 10:44

akudiyar changed the title ~~Change sync realisation on async in retrying~~ Change sync implementation on async in retrying Dec 29, 2022

ArtDu force-pushed the retrying branch from 96b9656 to 5c5326f Compare December 29, 2022 11:27

akudiyar requested changes Dec 29, 2022

View reviewed changes

ArtDu force-pushed the retrying branch 2 times, most recently from 5d450ea to c7bd439 Compare December 29, 2022 15:25

akudiyar requested changes Dec 29, 2022

View reviewed changes

akudiyar requested changes Jan 9, 2023

View reviewed changes

ArtDu force-pushed the retrying branch 2 times, most recently from c13c011 to d736235 Compare January 10, 2023 09:47

ArtDu requested a review from Elishtar January 10, 2023 10:20

Elishtar previously approved these changes Jan 10, 2023

View reviewed changes

bitgorbovsky previously approved these changes Jan 10, 2023

View reviewed changes

ArtDu dismissed stale reviews from bitgorbovsky and Elishtar via 1eaa40b January 10, 2023 13:38

ArtDu force-pushed the retrying branch from c8defcc to 6ba2e08 Compare January 10, 2023 14:03

ArtDu mentioned this pull request Jan 10, 2023

Use Runnable classes in AsyncOperation and TimeoutOperation in retrying #341

Closed

ArtDu requested a review from akudiyar January 11, 2023 07:43

akudiyar previously approved these changes Jan 11, 2023

View reviewed changes

ArtDu dismissed akudiyar’s stale review via 6d80a5d January 12, 2023 08:00

ArtDu force-pushed the retrying branch 2 times, most recently from 6d80a5d to e6c3c57 Compare January 12, 2023 08:03

Change sync realisation on async in retrying

8bf8d37

Closes #299

ArtDu force-pushed the retrying branch from e6c3c57 to 8bf8d37 Compare January 12, 2023 08:46

bitgorbovsky approved these changes Jan 12, 2023

View reviewed changes

Elishtar approved these changes Jan 12, 2023

View reviewed changes

ArtDu merged commit 83841c4 into master Jan 12, 2023

ArtDu deleted the retrying branch January 12, 2023 09:17

Conversation

ArtDu commented Dec 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

bitgorbovsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

akudiyar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ArtDu Jan 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ArtDu Jan 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akudiyar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ArtDu commented Dec 26, 2022 •

edited

Loading

ArtDu Jan 1, 2023 •

edited

Loading

ArtDu Jan 10, 2023 •

edited

Loading