Copilot commented on code in PR #4883:
URL: https://github.com/apache/bookkeeper/pull/4883#discussion_r3974638310
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerHandle.java:
##########
@@ -213,7 +213,8 @@ private enum HandleState {
this.pendingAddsSequenceHead = lastAddConfirmed;
this.ledgerId = ledgerId;
- this.executor = clientCtx.getMainWorkerPool().chooseThread(ledgerId);
+ // The main worker pool is a plain OrderedExecutor, whose threads are
SingleThreadExecutor instances
+ this.executor = (SingleThreadExecutor)
clientCtx.getMainWorkerPool().chooseThread(ledgerId);
Review Comment:
This hard cast can throw `ClassCastException` at runtime if
`chooseThread(...)` returns an `ExecutorService` implementation that is not a
`SingleThreadExecutor` (e.g., a wrapper/decorator or a different pool
implementation). Suggestion (mandatory): avoid the hard cast by either (a)
storing the chosen executor as `ExecutorService` and using `executeOrRun` only
when `instanceof SingleThreadExecutor`, otherwise falling back to the ordered
submission path, or (b) enforcing/validating the type with a clear failure mode
(explicit check + informative exception) rather than a raw cast.
##########
bookkeeper-common/src/main/java/org/apache/bookkeeper/common/util/SingleThreadExecutor.java:
##########
@@ -220,6 +231,30 @@ public void execute(Runnable r) {
executeRunnableOrList(r, null);
}
+ /**
+ * Whether the calling thread is the thread of this executor.
+ */
+ public boolean isCurrentThread() {
+ return Thread.currentThread() == runner;
+ }
+
+ /**
+ * Runs the task inline when called from this executor's own thread,
otherwise submits it like
+ * {@link #execute(Runnable)}.
+ *
+ * <p>The inline run bypasses the queue: a task submitted this way from
the executor thread runs before the
+ * tasks already queued, nested inside the task that submitted it. Use it
only where that reordering is
+ * acceptable. Failures are logged and counted like those of queued tasks.
+ */
+ public void executeOrRun(Runnable r) {
+ if (isCurrentThread()) {
+ tasksCount.increment();
+ safeRunTask(r);
+ } else {
+ execute(r);
+ }
+ }
Review Comment:
`executeOrRun` bypasses the executor state checks that `execute(...)`
performs via `executeRunnableOrList(...)`. As written, a task can still run
inline after shutdown (or during non-Running states) if called from the runner
thread, which violates the `ExecutorService` contract and makes shutdown
semantics inconsistent. Suggestion (mandatory): add the same state gate used by
`executeRunnableOrList(...)` (e.g., throw `RejectedExecutionException` when not
`Running`) before taking the inline path.
--
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]
