Split retrying into more detailed modules by ArtDu · Pull Request #340 · tarantool/cartridge-java

ArtDu · 2023-01-10T12:33:04Z

I haven't forgotten about:

Related issues:
Closes #341

akudiyar

Overall looks better than the current state, let's comb it out a bit

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

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

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

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

akudiyar · 2023-01-22T21:47:43Z

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

+                new RetryingAsyncOperation<>(this, operation, resultFuture, lastExceptionWrapper);
            CompletableFuture.runAsync(() -> {
-                    runAsyncOperation(operation, resultFuture, lastExceptionWrapper);
+                    retryingAsyncOperation.run();


Wouldn't it be better here to organize the code as
.runAsync(retryingAsyncOperation, executor).acceptEither(... global timeout future..., ...dummy callback...) ?

The current variant does not schedule the global timeout future correctly, because the "run()" method is executed synchronously in the current thread

And we need to start operation asynchronously because we need to start timer after starting of the task

But you are first finishing the task here and then scheduling a timer. You need to run the main task asynchronously.

But you are first finishing the task here and then scheduling a timer. You need to run the main task asynchronously.

I don't finish the task here, I just start it. Using acceptEither could give us the probability of completing the timeout task before the operation was started

It should never happen really. Both tasks are scheduled with acceptEither.

Yeah, it will wait for them until they both finish. But retryingAsyncOperation.run(); is not running of the operation, it's just starting of operation. Then the scheduler can start timeout before operation

runAsync is not running the operation too, it just schedules it in the executor. We schedule both tasks and wait on both, which one finishes first:
https://www.concretepage.com/java/java-8/java-completablefuture-accepteither
Note, we may need to use supplyAsync instead of runAsync, depending on the callback.

You already have a good example in your code BTW: https://github.com/tarantool/cartridge-java/pull/340/files#diff-8c78be7b6b292540e567ac6d0bd09e022bb23e553a11b407d95ed3dcb2709dbdR50

This code will be cleaner, although I admit run()" is not "synchronous" in the way that we get the operation result first. But we may call the callback in whenComplete() there even before we start the timeout.

akudiyar · 2023-01-22T21:50:43Z

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

-                    }
-                    return null;
-                }).exceptionally(ex -> { // if error has been happened in previous exceptionally section
+        public <R> CompletableFuture<R> wrapOperation(Supplier<CompletableFuture<R>> operation, Executor executor) {


It seems that this function can now be combined with the one in the base class and does not need a separate implementation

It was combined if I remember it correcty

akudiyar

Answered some of the comments

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

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

akudiyar · 2023-01-31T20:52:36Z

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

+        // start async operation running
+        CompletableFuture<T> operationFuture = operation.get();
+        // start scheduled request timeout task
+        // it never completes correctly only exceptionally


We still need to either delete the comment (it is duplicated by the class Javadoc) or rewrite it to the timeout task can only be completed with an exception or canceled for example.

akudiyar · 2023-01-31T20:53:37Z

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

+        CompletableFuture<T> requestTimeoutFuture = requestTimeoutOperation.get();
+
+        operationFuture.acceptEither(requestTimeoutFuture, resultFuture::complete)
+            .exceptionally(ex -> { // if requestTimeout has been raised or operation return exception


But it will be the only one, instead of several unnecessary return null statements.

akudiyar · 2023-01-31T20:55:28Z

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

+
+                if (policy.canRetryRequest(ex)) {
+                    // retry it after delay
+                    ScheduledFuture<?> delayFuture = TarantoolRequestRetryPolicies.getTimeoutScheduler()


Please add any description to the error. Otherwise, anyone will need to dig down the sources for the source code line.

akudiyar · 2023-01-31T20:58:49Z

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

+                    resultFuture.completeExceptionally(ex);
+                }
+                return null;
+            }).exceptionally(ex -> { // if error has been happened in previous exceptionally section


If try ... except is within a callback, it's a normal practice and even the only way in some cases (like for the checked exceptions).

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

akudiyar · 2023-01-31T21:02:37Z

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

+                new RetryingAsyncOperation<>(this, operation, resultFuture, lastExceptionWrapper);
            CompletableFuture.runAsync(() -> {
-                    runAsyncOperation(operation, resultFuture, lastExceptionWrapper);
+                    retryingAsyncOperation.run();


But you are first finishing the task here and then scheduling a timer. You need to run the main task asynchronously.

Closes #341 Co-authored-by: Alexey Kuzin <akudiyar@gmail.com>

dkasimovskiy · 2023-03-10T07:47:13Z

@akudiyar please take a look on the PR.

akudiyar

It seems that some changes that I was reviewing disappeared completely. so I think we need to merge this already. Nvm these comments, but let them be

akudiyar · 2023-03-03T23:03:45Z

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

+                new RetryingAsyncOperation<>(this, operation, resultFuture, lastExceptionWrapper);
            CompletableFuture.runAsync(() -> {
-                    runAsyncOperation(operation, resultFuture, lastExceptionWrapper);
+                    retryingAsyncOperation.run();


You already have a good example in your code BTW: https://github.com/tarantool/cartridge-java/pull/340/files#diff-8c78be7b6b292540e567ac6d0bd09e022bb23e553a11b407d95ed3dcb2709dbdR50

akudiyar · 2023-03-03T23:10:37Z

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

+                new RetryingAsyncOperation<>(this, operation, resultFuture, lastExceptionWrapper);
            CompletableFuture.runAsync(() -> {
-                    runAsyncOperation(operation, resultFuture, lastExceptionWrapper);
+                    retryingAsyncOperation.run();


This code will be cleaner, although I admit run()" is not "synchronous" in the way that we get the operation result first. But we may call the callback in whenComplete() there even before we start the timeout.

ArtDu force-pushed the retrying_runnable branch from 5169d53 to d5febfe Compare January 10, 2023 12:45

ArtDu mentioned this pull request Jan 10, 2023

Change sync implementation on async in retrying #333

Merged

5 tasks

ArtDu force-pushed the retrying_runnable branch from d5febfe to 9409a8d Compare January 12, 2023 15:12

akudiyar requested changes Jan 22, 2023

View reviewed changes

ArtDu force-pushed the retrying_runnable branch 3 times, most recently from 76958bb to 2721d58 Compare January 31, 2023 15:51

akudiyar requested changes Jan 31, 2023

View reviewed changes

ArtDu force-pushed the retrying_runnable branch 3 times, most recently from d1e379e to 1428b81 Compare March 1, 2023 11:52

ArtDu marked this pull request as ready for review March 1, 2023 11:59

ArtDu changed the title ~~WIP: Retrying runnable~~ Split retrying into more detailed modules Mar 1, 2023

ArtDu requested a review from akudiyar March 1, 2023 12:00

Decompose logic from retrying section

0175919

Closes #341 Co-authored-by: Alexey Kuzin <akudiyar@gmail.com>

ArtDu force-pushed the retrying_runnable branch from 1428b81 to 0175919 Compare March 1, 2023 12:05

akudiyar approved these changes Mar 13, 2023

View reviewed changes

akudiyar added this pull request to the merge queue Mar 13, 2023

Merged via the queue into master with commit d3a3d6a Mar 13, 2023

akudiyar deleted the retrying_runnable branch March 13, 2023 22:19

Conversation

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

Uh oh!

Uh oh!

akudiyar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

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!

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

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dkasimovskiy commented Mar 10, 2023

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!

3 participants

ArtDu commented Jan 10, 2023 •

edited

Loading