Introduce manual job restart #693
Conversation
|
Test FAILed. |
| boolean cancelCurrentExecution() { | ||
| ExecutionCallback<Object> callback = this.executionCancellationCallback; | ||
| if (callback != null) { | ||
| callback.onFailure(null); |
There was a problem hiding this comment.
we should add a comment here. Do you think having an explict exception would be better? I.e. JobRestartRequestedException ? Also naming is still a bit ambiguous between cancelExecute and cancelCurrentExecution`.
| invoke(operationCtor, this::onExecuteStepCompleted, callback); | ||
| Consumer<Map<MemberInfo, Object>> completionCallback = results -> { | ||
| executionCancellationCallback = null; | ||
| onExecuteStepCompleted(results); |
There was a problem hiding this comment.
I can see that the job is only restarted when it's TopologyChangedException and auto-restart is enabled, otherwise a restart will not be scheduled.
|
verify |
|
Test PASSed. |
|
verify |
|
Test PASSed. |
1 similar comment
|
Test PASSed. |
| * Cancels the execution invocation in order to restart it afterwards if the job is currently being executed | ||
| */ | ||
| boolean cancelCurrentExecution() { | ||
| CompletionToken restartFuture = this.executionRestartToken; |
|
|
||
| /** | ||
| * Wraps a CompletableFuture that is expected to be completed | ||
| * only in a single way and provides methods for easy of use |
| } | ||
|
|
||
| public void whenCompleted(Runnable runnable) { | ||
| future.whenComplete((result, throwable) -> runnable.run()); |
There was a problem hiding this comment.
it should use withTryCatch here, otherwise any exceptions in callback are lost and won't be visible.
| private final CompletableFuture<Void> future = new CompletableFuture<>(); | ||
|
|
||
| public boolean complete() { | ||
| return completeVoidFuture(future); |
There was a problem hiding this comment.
this method is now deleted after spotbugs change, you need to rebase
|
Test PASSed. |
metanet
force-pushed
the
elasticity-pr
branch
from
aca836a
to
f395595
Compare
|
Test FAILed. |
|
verify |
|
Test PASSed. |
metanet
force-pushed
the
elasticity-pr
branch
from
f395595
to
ef7a143
Compare
|
Test PASSed. |
| /** | ||
| * Cancels the job execution invocations in order to restart it afterwards if the job is currently being executed | ||
| */ | ||
| boolean cancelCurrentExecution() { |
There was a problem hiding this comment.
You use the terms "restart" and "cancel current execution" for the same thing. I would be good to use one, "restart" is easier to understand for me.
|
|
||
| // We must set executionRestartToken before we call invoke() method because once all invocations | ||
| // are done, executionRestartToken will be reset. Therefore, setting it after the invoke() call is racy. | ||
| this.executionRestartToken = executionRestartFuture; |
There was a problem hiding this comment.
The local variable could also be named executionRestartToken
| * | ||
| * @throws IllegalStateException if the job has been already completed | ||
| * | ||
| * @return true if the current execution of the job is cancelled |
There was a problem hiding this comment.
It would be good to mention when false is returned. I guess it's when job is not yet started or is already restarting.
There was a problem hiding this comment.
We already mention when it returns true: "Cancels the current execution if the job is currently in running". Do we need to mention when it returns false?
There was a problem hiding this comment.
Now I got your point. But anyway, i mean it would be good to mention it in the @return clause.
A new method is added to the Job interface. If the user calls job.restart() while a job is running, the job is restarted on the current member list of the cluster from the last successful snapshot. The new method can be used as a non-graceful way of achieving elasticity.
metanet
force-pushed
the
elasticity-pr
branch
from
6c23a78
to
ec87806
Compare
|
Test PASSed. |
2 similar comments
|
Test PASSed. |
|
Test PASSed. |
A new method is added to the Job interface. If the user calls job.restart() while a job is running, the job is restarted on the current member list of the cluster from the last successful snapshot. The new method can be used as a non-graceful way of achieving elasticity.