Improve waiting strategy for Integration tests - cont #409
Conversation
| if (System.currentTimeMillis() >= endTime) { | ||
| future.completeExceptionally(new TimeoutException("Retry limit reached.")); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Set a timeout in future.get() (i.e. future.get(endTime, TimeUnit.MILLISECONDS); does it have equivalent behavior to this block of code?
There was a problem hiding this comment.
No, setting a timeout using future.get(endTime, TimeUnit.MILLISECONDS) does not provide an equivalent behavior to the previous block of code in the RetryExecutorService.
The future.get(endTime, TimeUnit.MILLISECONDS) method call sets a timeout for waiting to retrieve the result from the future. If the result is not available within the specified timeout period, a TimeoutException will be thrown. However, it does not cancel the ongoing execution of the task or stop further retry attempts.
sdk-utils/src/main/java/com/redhat/parodos/sdkutils/RetryExecutorService.java
Outdated
Show resolved
Hide resolved
The PR replaces the former busy-wait implementation with a scheduled executor service. With this implementation, a scheduled service will try invoking the given callback (for querying projects or waiting on a specific workflow status) for up to 2 minutes. Signed-off-by: Moti Asayag <masayag@redhat.com>
The PR modifies the notification service integration tests to use the same RetryExecutorService class for waiting on the notification service to become available. Signed-off-by: Moti Asayag <masayag@redhat.com>
Signed-off-by: Moti Asayag <masayag@redhat.com>
| @@ -24,7 +28,7 @@ public RetryExecutorService() { | |||
| */ | |||
| public T submitWithRetry(Callable<T> task) { | |||
| // @formatter:off | |||
| return submitWithRetry(task, () -> {}, () -> {}, 2 * 60 * 1000, 5000); | |||
| return submitWithRetry(task, () -> {}, () -> {}, MAX_RETRY_TIME, RETRY_DELAY); | |||
There was a problem hiding this comment.
I like this (and the commit message ;) )
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gciavarrini The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
| Future<T> future = executor.submit(() -> { | ||
| long startTime = System.currentTimeMillis(); | ||
| long endTime = startTime + maxRetryTime; | ||
| CompletableFuture<T> future = new CompletableFuture<>(); |
There was a problem hiding this comment.
why is this future here needed?
There was a problem hiding this comment.
it stores the result and is used as a reference to its success or failure.
there are 2 "futures" here: one is responsible for the retries which are the scheduledFuture.
the other one is used to report the result in line 67.
would you suggest a different method of doing it?
What this PR does / why we need it:
The PR replaces the former busy-wait implementation with a scheduled executor service.
With this implementation, a scheduled service will try invoking the given callback (for querying projects or waiting on a specific workflow status) for up to 2 minutes.
The PR also modifies the notification service integration tests to use the same RetryExecutorService class for waiting on the notification service to become available.
Which issue(s) this PR fixes (optional, use
fixes #<issue_number>(, fixes #<issue_number>, ...)format, where issue_number might be a GitHub issue, or a Jira story (FLPATH-xxxx):Fixes #FLPATH-399
Change type
Impacted services
Checklist