dimas-b commented on code in PR #4504:
URL: https://github.com/apache/polaris/pull/4504#discussion_r3284173070
##########
persistence/nosql/async/vertx/src/main/java/org/apache/polaris/nosql/async/vertx/VertxAsyncExec.java:
##########
@@ -226,28 +263,27 @@ void execAsync(long unused) {
return;
}
// Skip overlapping periodic executions
- if (periodic && running.get()) {
+ if (periodic && !running.compareAndSet(false, true)) {
return;
}
try {
var f = executorService.submit(this);
runningFuture.set(f);
- } catch (RejectedExecutionException rex) {
- completable.completeExceptionally(rex);
+ VertxAsyncExec.this.taskSubmittedHook(f);
+ } catch (Throwable e) {
+ completable.completeExceptionally(e);
+ running.set(false);
Review Comment:
Might be best to reset `running` first.
##########
persistence/nosql/async/vertx/src/main/java/org/apache/polaris/nosql/async/vertx/VertxAsyncExec.java:
##########
@@ -189,34 +214,46 @@ private final class CancelableFuture<R> implements
Cancelable<R>, Runnable {
// Track in-flight submission to allow cancelling/interrupting
private final AtomicReference<Future<?>> runningFuture = new
AtomicReference<>();
- CancelableFuture(Runnable runnable, long initialMillis, long delayMillis) {
- initialMillis = Math.max(initialMillis, 1L);
- delayMillis = Math.max(delayMillis, 1L);
+ /** Constructor for periodic tasks. */
+ CancelableFuture(Runnable runnable) {
this.runnable = requireNonNull(runnable, "Runnable must not be null");
- this.callable = this::runnable;
- this.timerId = vertx.setPeriodic(initialMillis, delayMillis,
this::execAsync);
- this.hasTimerId = true;
+ this.callable =
+ () -> {
+ runnable.run();
+ return null;
+ };
+ this.timerState.set(NotStarted.INSTANCE);
this.periodic = true;
}
- CancelableFuture(Callable<R> callable, long delayMillis) {
+ /** Constructor for one-shot tasks. */
+ CancelableFuture(Callable<R> callable, boolean delayed) {
this.runnable = null;
this.callable = requireNonNull(callable, "Callable must not be null");
this.periodic = false;
- if (delayMillis > 0) {
- this.hasTimerId = true;
- this.timerId = vertx.setTimer(delayMillis, this::execAsync);
- } else {
- // Execute immediately
- this.hasTimerId = false;
- this.timerId = 0L;
+ this.timerState.set(delayed ? NotStarted.INSTANCE : Canceled.INSTANCE);
Review Comment:
Does `?` matter here? Why not always `NotStarted`? The `delayed` value here
and `delayMillis` in `startScheduled()` come from controlled code anyway. If we
cannot trust those values to be in sync, this condition does not help much, I
guess 🤔
##########
persistence/nosql/async/vertx/src/main/java/org/apache/polaris/nosql/async/vertx/VertxAsyncExec.java:
##########
@@ -189,34 +214,46 @@ private final class CancelableFuture<R> implements
Cancelable<R>, Runnable {
// Track in-flight submission to allow cancelling/interrupting
private final AtomicReference<Future<?>> runningFuture = new
AtomicReference<>();
- CancelableFuture(Runnable runnable, long initialMillis, long delayMillis) {
- initialMillis = Math.max(initialMillis, 1L);
- delayMillis = Math.max(delayMillis, 1L);
+ /** Constructor for periodic tasks. */
+ CancelableFuture(Runnable runnable) {
this.runnable = requireNonNull(runnable, "Runnable must not be null");
- this.callable = this::runnable;
- this.timerId = vertx.setPeriodic(initialMillis, delayMillis,
this::execAsync);
- this.hasTimerId = true;
+ this.callable =
+ () -> {
+ runnable.run();
+ return null;
+ };
+ this.timerState.set(NotStarted.INSTANCE);
Review Comment:
same as default?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
