[GitHub] [incubator-druid] leventov commented on a change in pull request #8809: Prohibit Futures.addCallback(Future, Callback)
leventov commented on a change in pull request #8809: Prohibit Futures.addCallback(Future, Callback) URL: https://github.com/apache/incubator-druid/pull/8809#discussion_r355116113 ## File path: indexing-service/src/main/java/org/apache/druid/indexing/seekablestream/SeekableStreamIndexTaskRunner.java ## @@ -219,6 +221,7 @@ private final List> handOffWaitList = new ArrayList<>(); private final LockGranularity lockGranularityToUse; + private final ExecutorService sequencePersistExecutor; Review comment: In classes that don't have `close()`/`stop()` methods yet, like `SeekableStreamIndexTaskRunner`, they should be introduced if you want to manage an executor. If the class is pretty ephemeral (e. g. many instances of this class routinely start and stop), it should be strongly preferred to create an executor one level up the stack and reuse it for all instances of the class (or all instances of the class within a certain domain e. g. all `SeekableStreamIndexTaskRunner`s associated with a single datasource, or something like that). Note again, I don't actually tell you that this class `SeekableStreamIndexTaskRunner` falls into this category - please figure it out yourself, or consult the maintainers of this subsystem. 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org For additional commands, e-mail: commits-h...@druid.apache.org
[GitHub] [incubator-druid] leventov commented on a change in pull request #8809: Prohibit Futures.addCallback(Future, Callback)
leventov commented on a change in pull request #8809: Prohibit
Futures.addCallback(Future, Callback)
URL: https://github.com/apache/incubator-druid/pull/8809#discussion_r355115743
##
File path:
indexing-service/src/main/java/org/apache/druid/indexing/overlord/RemoteTaskRunner.java
##
@@ -350,6 +350,7 @@ public void stop()
return;
}
try {
+ monitorSyncHandler.shutdown();
Review comment:
These two things must be closed using the `closer` below
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org
For additional commands, e-mail: commits-h...@druid.apache.org
[GitHub] [incubator-druid] leventov commented on a change in pull request #8809: Prohibit Futures.addCallback(Future, Callback)
leventov commented on a change in pull request #8809: Prohibit Futures.addCallback(Future, Callback) URL: https://github.com/apache/incubator-druid/pull/8809#discussion_r355115822 ## File path: indexing-service/src/main/java/org/apache/druid/indexing/overlord/TaskQueue.java ## @@ -219,6 +219,7 @@ public void stop() tasks.clear(); taskFutures.clear(); active = false; + statusHandler.shutdownNow(); Review comment: Please shutdown these three Executors using a `Closer` 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org For additional commands, e-mail: commits-h...@druid.apache.org
[GitHub] [incubator-druid] leventov commented on a change in pull request #8809: Prohibit Futures.addCallback(Future, Callback)
leventov commented on a change in pull request #8809: Prohibit Futures.addCallback(Future, Callback) URL: https://github.com/apache/incubator-druid/pull/8809#discussion_r354940018 ## File path: indexing-service/src/main/java/org/apache/druid/indexing/seekablestream/SeekableStreamIndexTaskRunner.java ## @@ -1008,7 +1014,8 @@ public void onFailure(Throwable t) log.error(t, "Error while publishing segments for sequenceNumber[%s]", sequenceMetadata); handoffFuture.setException(t); } -} +}, +sequencePersistExecutor Review comment: This still needs to be reviewed by maintainers of `SeekableStreamIndexTaskRunner`. 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org For additional commands, e-mail: commits-h...@druid.apache.org
[GitHub] [incubator-druid] leventov commented on a change in pull request #8809: Prohibit Futures.addCallback(Future, Callback)
leventov commented on a change in pull request #8809: Prohibit Futures.addCallback(Future, Callback) URL: https://github.com/apache/incubator-druid/pull/8809#discussion_r354939431 ## File path: server/src/main/java/org/apache/druid/segment/realtime/appenderator/AppenderatorPlumber.java ## @@ -483,7 +485,8 @@ public void onFailure(Throwable e) log.warn(e, "Failed to push [%,d] segments.", segmentsToPush.size()); errorHandler.apply(e); } -} +}, +scheduledExecutor Review comment: This still needs to be reviewed by maintainers of `AppenderatorPlumber` 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org For additional commands, e-mail: commits-h...@druid.apache.org
[GitHub] [incubator-druid] leventov commented on a change in pull request #8809: Prohibit Futures.addCallback(Future, Callback)
leventov commented on a change in pull request #8809: Prohibit Futures.addCallback(Future, Callback) URL: https://github.com/apache/incubator-druid/pull/8809#discussion_r354938025 ## File path: indexing-service/src/main/java/org/apache/druid/indexing/overlord/RemoteTaskRunner.java ## @@ -175,6 +175,7 @@ private final ConcurrentMap removedWorkerCleanups = new ConcurrentHashMap<>(); private final ProvisioningStrategy provisioningStrategy; + private final ExecutorService monitorSyncHandler; Review comment: This extra service must be explicitly shutdown in `stop()`/`close()`, see https://github.com/code-review-checklists/java-concurrency#explicit-shutdown. Applies to other classes in this PR as well 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org For additional commands, e-mail: commits-h...@druid.apache.org
[GitHub] [incubator-druid] leventov commented on a change in pull request #8809: Prohibit Futures.addCallback(Future, Callback)
leventov commented on a change in pull request #8809: Prohibit Futures.addCallback(Future, Callback) URL: https://github.com/apache/incubator-druid/pull/8809#discussion_r347088004 ## File path: indexing-service/src/main/java/org/apache/druid/indexing/overlord/RemoteTaskRunner.java ## @@ -257,7 +257,7 @@ public void onFailure(Throwable throwable) } } }, -// The callback is non-blocking and quick, so it's OK to schedule it using directExecutor() +// The callback is mostly non-blocking and quick, so it's OK to schedule it using directExecutor() Review comment: [In my comment](https://github.com/apache/incubator-druid/pull/8809#discussion_r342940649), I wrote "*you may argue that*... this code is mostly non-blocking", I didn't *state* that (and I actually don't know). I. e. the fact that the synchronized block in `onSuccess()` requires some proof, expressed in a comment. There is not enough information to make such proof (or disproof) just in this code excerpt in Github's interface. A wider context (specifically, the code which synchronizes on `waitingForMonitor` and waits) should be considered. 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org For additional commands, e-mail: commits-h...@druid.apache.org
[GitHub] [incubator-druid] leventov commented on a change in pull request #8809: Prohibit Futures.addCallback(Future, Callback)
leventov commented on a change in pull request #8809: Prohibit Futures.addCallback(Future, Callback) URL: https://github.com/apache/incubator-druid/pull/8809#discussion_r347088068 ## File path: server/src/main/java/org/apache/druid/server/http/SegmentListerResource.java ## @@ -205,7 +209,8 @@ public void onFailure(Throwable th) log.debug(ex, "Request timed out or closed already."); } } -} +}, Review comment: Ok, but the comments that you left right here don't make anything clearer. It's ok to omit them in this PR and instead write better comments as part of that PR that you planned, though. 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org For additional commands, e-mail: commits-h...@druid.apache.org
[GitHub] [incubator-druid] leventov commented on a change in pull request #8809: Prohibit Futures.addCallback(Future, Callback)
leventov commented on a change in pull request #8809: Prohibit Futures.addCallback(Future, Callback) URL: https://github.com/apache/incubator-druid/pull/8809#discussion_r347087847 ## File path: indexing-service/src/main/java/org/apache/druid/indexing/worker/WorkerTaskManager.java ## @@ -177,6 +177,9 @@ public void stop() throws Exception return completedTasks; } + /** + * This method is mostly non-blocking because exec is a ThreadPoolExecutor with unbounded queue used by default. Review comment: Please make `exec` and `ThreadPoolExecutor` javadoc links 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org For additional commands, e-mail: commits-h...@druid.apache.org
[GitHub] [incubator-druid] leventov commented on a change in pull request #8809: Prohibit Futures.addCallback(Future, Callback)
leventov commented on a change in pull request #8809: Prohibit Futures.addCallback(Future, Callback) URL: https://github.com/apache/incubator-druid/pull/8809#discussion_r342951053 ## File path: indexing-service/src/main/java/org/apache/druid/indexing/common/task/batch/parallel/ParallelIndexPhaseRunner.java ## @@ -250,7 +254,8 @@ public void onFailure(Throwable t) LOG.error(t, "Error while running a task for subTaskSpec[%s]", spec); taskCompleteEvents.offer(SubTaskCompleteEvent.fail(spec, t)); } -} +}, +blockingQueueHandler Review comment: Correction - `BlockingQueue.offer()` is still needs to enter a critical section, but it may be considered "mostly non-blocking", so directExecutor() should still be OK 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org For additional commands, e-mail: commits-h...@druid.apache.org
[GitHub] [incubator-druid] leventov commented on a change in pull request #8809: Prohibit Futures.addCallback(Future, Callback)
leventov commented on a change in pull request #8809: Prohibit Futures.addCallback(Future, Callback) URL: https://github.com/apache/incubator-druid/pull/8809#discussion_r342951053 ## File path: indexing-service/src/main/java/org/apache/druid/indexing/common/task/batch/parallel/ParallelIndexPhaseRunner.java ## @@ -250,7 +254,8 @@ public void onFailure(Throwable t) LOG.error(t, "Error while running a task for subTaskSpec[%s]", spec); taskCompleteEvents.offer(SubTaskCompleteEvent.fail(spec, t)); } -} +}, +blockingQueueHandler Review comment: Correction - `BlockingQueue.offer()` still needs to enter a critical section, but it may be considered "mostly non-blocking", so directExecutor() should still be OK 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org For additional commands, e-mail: commits-h...@druid.apache.org
[GitHub] [incubator-druid] leventov commented on a change in pull request #8809: Prohibit Futures.addCallback(Future, Callback)
leventov commented on a change in pull request #8809: Prohibit
Futures.addCallback(Future, Callback)
URL: https://github.com/apache/incubator-druid/pull/8809#discussion_r342947361
##
File path:
indexing-service/src/main/java/org/apache/druid/indexing/worker/WorkerTaskManager.java
##
@@ -251,7 +251,9 @@ public void onFailure(Throwable t)
{
submitNoticeToExec(new StatusNotice(task,
TaskStatus.failure(task.getId(;
}
-}
+},
+// The callback is non-blocking and quick, so it's OK to schedule it
using directExecutor()
Review comment:
Please add a javadoc comment to `submitNoticeToExec()` like "This method is
mostly non-blocking because exec is a ThreadPoolExecutor with unbounded queue
used by default."
Correspondingly, this comment also should say "mostly non-blocking", not
just "non-blocking".
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org
For additional commands, e-mail: commits-h...@druid.apache.org
[GitHub] [incubator-druid] leventov commented on a change in pull request #8809: Prohibit Futures.addCallback(Future, Callback)
leventov commented on a change in pull request #8809: Prohibit
Futures.addCallback(Future, Callback)
URL: https://github.com/apache/incubator-druid/pull/8809#discussion_r342940087
##
File path:
core/src/main/java/org/apache/druid/java/util/common/concurrent/ListenableFutures.java
##
@@ -61,15 +61,19 @@ public void onFailure(Throwable t)
{
finalFuture.setException(t);
}
-});
+},
+// The callback is non-blocking and quick, so it's OK to schedule
it using directExecutor()
Review comment:
Too much indentation
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org
For additional commands, e-mail: commits-h...@druid.apache.org
[GitHub] [incubator-druid] leventov commented on a change in pull request #8809: Prohibit Futures.addCallback(Future, Callback)
leventov commented on a change in pull request #8809: Prohibit
Futures.addCallback(Future, Callback)
URL: https://github.com/apache/incubator-druid/pull/8809#discussion_r342940115
##
File path:
core/src/main/java/org/apache/druid/java/util/common/concurrent/ListenableFutures.java
##
@@ -61,15 +61,19 @@ public void onFailure(Throwable t)
{
finalFuture.setException(t);
}
-});
+},
+// The callback is non-blocking and quick, so it's OK to schedule
it using directExecutor()
+Execs.directExecutor());
}
@Override
public void onFailure(Throwable t)
{
finalFuture.setException(t);
}
-});
+},
+// The callback is non-blocking and quick, so it's OK to schedule it
using directExecutor()
Review comment:
Same
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org
For additional commands, e-mail: commits-h...@druid.apache.org
[GitHub] [incubator-druid] leventov commented on a change in pull request #8809: Prohibit Futures.addCallback(Future, Callback)
leventov commented on a change in pull request #8809: Prohibit
Futures.addCallback(Future, Callback)
URL: https://github.com/apache/incubator-druid/pull/8809#discussion_r342945408
##
File path:
indexing-service/src/main/java/org/apache/druid/indexing/overlord/TaskQueue.java
##
@@ -556,7 +559,8 @@ private void handleStatus(final TaskStatus status)
.emit();
}
}
-}
+},
+statusHandler
Review comment:
It's non-obvious from this point in code to see that `attachCallbacks()` is
called from a different thread pool than `statusFuture` is completed.
There are options to improve:
- Propagate `Executor` argument and pass `statusHandler` only in
`manage()`, and, additionally, amend the javadoc comment for `manage()` noting
that it is run from `{@link #managerExec}`.
- Just add a more elaborate comment right here.
In either case, it will be non-obvious for readers why different executors
demand a special executor. So the comment should look something like that:
"Using dedicated statusHandler executor instead of directExecutor() because
the callback's onSuccess() is not trivial and because the statusFuture is
completed in some incapsulated thread pool in TaskRunner; directExecutor() may
create operational instability and subtle dependency between components here.
See
https://github.com/code-review-checklists/java-concurrency#cf-beware-non-async
for details."
Something similar should be done in all other places where you use a custom
executor instead of directExecutor().
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org
For additional commands, e-mail: commits-h...@druid.apache.org
[GitHub] [incubator-druid] leventov commented on a change in pull request #8809: Prohibit Futures.addCallback(Future, Callback)
leventov commented on a change in pull request #8809: Prohibit Futures.addCallback(Future, Callback) URL: https://github.com/apache/incubator-druid/pull/8809#discussion_r342948998 ## File path: server/src/main/java/org/apache/druid/server/http/SegmentListerResource.java ## @@ -205,7 +209,8 @@ public void onFailure(Throwable th) log.debug(ex, "Request timed out or closed already."); } } -} +}, Review comment: Same. This code looks similar to `TaskManagementResource`. Maybe the common logic should be extracted. Or, there should be comments to both of them noting that these classes are written in a similar way so changes, fixes, refactorings should be applied to both classes at the same time. 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org For additional commands, e-mail: commits-h...@druid.apache.org
[GitHub] [incubator-druid] leventov commented on a change in pull request #8809: Prohibit Futures.addCallback(Future, Callback)
leventov commented on a change in pull request #8809: Prohibit Futures.addCallback(Future, Callback) URL: https://github.com/apache/incubator-druid/pull/8809#discussion_r342948270 ## File path: indexing-service/src/main/java/org/apache/druid/indexing/worker/http/TaskManagementResource.java ## @@ -192,7 +196,8 @@ public void onFailure(Throwable th) log.debug(ex, "Request timed out or closed already."); } } -} +}, Review comment: Some weird asynchrony going on in this method (at least it looks so), so it demands more comments 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org For additional commands, e-mail: commits-h...@druid.apache.org
[GitHub] [incubator-druid] leventov commented on a change in pull request #8809: Prohibit Futures.addCallback(Future, Callback)
leventov commented on a change in pull request #8809: Prohibit Futures.addCallback(Future, Callback) URL: https://github.com/apache/incubator-druid/pull/8809#discussion_r342945993 ## File path: indexing-service/src/main/java/org/apache/druid/indexing/seekablestream/SeekableStreamIndexTaskRunner.java ## @@ -1008,7 +1014,8 @@ public void onFailure(Throwable t) log.error(t, "Error while publishing segments for sequenceNumber[%s]", sequenceMetadata); handoffFuture.setException(t); } -} +}, +sequencePersistExecutor Review comment: I withdraw from analyzing execution flows and thread pool relationships here. @jihoonson could you please vet this? 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org For additional commands, e-mail: commits-h...@druid.apache.org
[GitHub] [incubator-druid] leventov commented on a change in pull request #8809: Prohibit Futures.addCallback(Future, Callback)
leventov commented on a change in pull request #8809: Prohibit Futures.addCallback(Future, Callback) URL: https://github.com/apache/incubator-druid/pull/8809#discussion_r342939947 ## File path: indexing-service/src/main/java/org/apache/druid/indexing/common/task/batch/parallel/ParallelIndexPhaseRunner.java ## @@ -250,7 +254,8 @@ public void onFailure(Throwable t) LOG.error(t, "Error while running a task for subTaskSpec[%s]", spec); taskCompleteEvents.offer(SubTaskCompleteEvent.fail(spec, t)); } -} +}, +blockingQueueHandler Review comment: `taskCompleteEvents.offer(completeEvent);` -- `BlockingQueue.offer()` is actually non-blocking 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org For additional commands, e-mail: commits-h...@druid.apache.org
[GitHub] [incubator-druid] leventov commented on a change in pull request #8809: Prohibit Futures.addCallback(Future, Callback)
leventov commented on a change in pull request #8809: Prohibit Futures.addCallback(Future, Callback) URL: https://github.com/apache/incubator-druid/pull/8809#discussion_r342940649 ## File path: indexing-service/src/main/java/org/apache/druid/indexing/overlord/RemoteTaskRunner.java ## @@ -256,7 +256,9 @@ public void onFailure(Throwable throwable) waitingForMonitor.notifyAll(); } } -} +}, +// The callback is non-blocking and quick, so it's OK to schedule it using directExecutor() Review comment: `synchronized (waitingForMonitor)` **is blocking**. You may argue that this lock is (almost) free, for some reasons, so then the callback should still be considered "mostly non-blocking" and OK to schedule on directExecutor(), but in any case it demands a more elaborate comment 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org For additional commands, e-mail: commits-h...@druid.apache.org
