Add SingleThreadExecutor.executeOrRun and issue reads inline from the ledger's own thread - #4883
Merged
merlimat merged 5 commits intoSep 11, 2026
Merged
Conversation
Add SingleThreadExecutor.isCurrentThread() and executeOrRun(Runnable): the task runs inline when called from the executor's own thread and is queued otherwise. The shortcut is opt-in because it bypasses the queue: a task submitted this way from the executor thread runs before the tasks already queued, nested inside the submitting task. execute() keeps its FIFO contract. Inline failures are logged and counted like those of queued tasks. LedgerHandle uses it for reads on a writable handle, which were queued onto the ledger's thread even when the caller was already on it. Adds already initiate inline on the caller's thread and reads on read-only handles already bypass the executor, so this was the remaining same-thread hop. Applications that run their own work on a thread from OrderedExecutor.chooseThread(...) can use the same check for their re-dispatches.
…-run path Review follow-ups: the inline path of SingleThreadExecutor.executeOrRun calls safeRunTask directly, so inline and queued tasks share the same failure isolation, logging and counters; the queued path only adds the pending-count decrement, which must not apply to inline runs. LedgerHandle keeps its worker thread as a SingleThreadExecutor instead of checking with instanceof: the client's main worker pool is a plain OrderedExecutor, whose threads are SingleThreadExecutor instances. The two test fixtures that handed the client an OrderedScheduler as its worker pool (MockClientContext, MockBookKeeperTestCase) now use an OrderedExecutor, with the mock bookie client dispatching on that same pool.
lhotari
approved these changes
Sep 10, 2026
Member
|
LGTM |
StevenLuMT
requested review from
eolivelli,
hezhangjian and
zymap
and
a lite review from Copilot
September 10, 2026 01:08
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Pull request overview
Adds an opt-in inline execution path for SingleThreadExecutor and uses it to avoid re-queuing read operations onto the same ledger worker thread when the caller is already running on that thread.
Changes:
- Add
SingleThreadExecutor.isCurrentThread()andexecuteOrRun(Runnable)plus supporting internal refactor to distinguish queued vs inline task execution. - Update
LedgerHandleto useexecuteOrRunfor writable-handle reads and type the per-ledger executor asSingleThreadExecutor. - Adjust test fixtures to use an
OrderedExecutoras the main worker pool and add tests validating inline read initiation and executor behavior.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| bookkeeper-common/src/main/java/org/apache/bookkeeper/common/util/SingleThreadExecutor.java | Adds inline execution API and refactors pending-count decrement to apply only to queued tasks. |
| bookkeeper-common/src/test/java/org/apache/bookkeeper/common/util/TestSingleThreadExecutor.java | Adds unit tests for isCurrentThread() and executeOrRun() ordering/counters/failure behavior. |
| bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerHandle.java | Uses executeOrRun for writable reads and casts chosen thread executor to SingleThreadExecutor. |
| bookkeeper-server/src/test/java/org/apache/bookkeeper/client/LedgerHandleInlineReadTest.java | New test validating that reads issued on ledger thread are initiated inline in the mock setup. |
| bookkeeper-server/src/test/java/org/apache/bookkeeper/client/MockClientContext.java | Switches main worker pool to OrderedExecutor and keeps OrderedScheduler for timers in tests. |
| bookkeeper-server/src/test/java/org/apache/bookkeeper/client/MockBookKeeperTestCase.java | Returns the new executor as the main worker pool in the mock client context. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…un after shutdown The scheduler line in MockClientContext.create was one character over the limit; the PR validation job runs checkstyle over all modules and failed on it. executeOrRun now rejects the task once the executor is shut down, like execute() does, instead of still running it inline when called from the executor thread.
…urrent-thread # Conflicts: # bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerHandle.java
OrderedExecutor wraps each worker thread in a forwarding decorator when task tracing or MDC preservation is enabled, so chooseThread() returned the wrapper rather than the SingleThreadExecutor and the cast in the LedgerHandle constructor failed for such clients. The auditor and the replication worker run with task tracing enabled in the tests, so every ledger open there failed with UnexpectedConditionException and re-replication never completed. Add the ThreadBoundExecutor interface (isCurrentThread, executeOrRun), implemented by SingleThreadExecutor and by the decorator, which applies the timing and MDC wrappers to inline runs as well. LedgerHandle keeps its executor as a ThreadBoundExecutor.
merlimat
added a commit
that referenced
this pull request
Sep 11, 2026
… ledger's own thread (#4883) * Run reads inline when issued from the ledger's own thread Add SingleThreadExecutor.isCurrentThread() and executeOrRun(Runnable): the task runs inline when called from the executor's own thread and is queued otherwise. The shortcut is opt-in because it bypasses the queue: a task submitted this way from the executor thread runs before the tasks already queued, nested inside the submitting task. execute() keeps its FIFO contract. Inline failures are logged and counted like those of queued tasks. LedgerHandle uses it for reads on a writable handle, which were queued onto the ledger's thread even when the caller was already on it. Adds already initiate inline on the caller's thread and reads on read-only handles already bypass the executor, so this was the remaining same-thread hop. Applications that run their own work on a thread from OrderedExecutor.chooseThread(...) can use the same check for their re-dispatches. * Type LedgerHandle.executor as SingleThreadExecutor and share the safe-run path Review follow-ups: the inline path of SingleThreadExecutor.executeOrRun calls safeRunTask directly, so inline and queued tasks share the same failure isolation, logging and counters; the queued path only adds the pending-count decrement, which must not apply to inline runs. LedgerHandle keeps its worker thread as a SingleThreadExecutor instead of checking with instanceof: the client's main worker pool is a plain OrderedExecutor, whose threads are SingleThreadExecutor instances. The two test fixtures that handed the client an OrderedScheduler as its worker pool (MockClientContext, MockBookKeeperTestCase) now use an OrderedExecutor, with the mock bookie client dispatching on that same pool. * Fix checkstyle line length in MockClientContext and reject executeOrRun after shutdown The scheduler line in MockClientContext.create was one character over the limit; the PR validation job runs checkstyle over all modules and failed on it. executeOrRun now rejects the task once the executor is shut down, like execute() does, instead of still running it inline when called from the executor thread. * Expose the thread identity through the OrderedExecutor decorators OrderedExecutor wraps each worker thread in a forwarding decorator when task tracing or MDC preservation is enabled, so chooseThread() returned the wrapper rather than the SingleThreadExecutor and the cast in the LedgerHandle constructor failed for such clients. The auditor and the replication worker run with task tracing enabled in the tests, so every ledger open there failed with UnexpectedConditionException and re-replication never completed. Add the ThreadBoundExecutor interface (isCurrentThread, executeOrRun), implemented by SingleThreadExecutor and by the decorator, which applies the timing and MDC wrappers to inline runs as well. LedgerHandle keeps its executor as a ThreadBoundExecutor.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Descriptions of the changes in this PR:
Adds an opt-in inline execution path to
SingleThreadExecutorand uses it for reads issued on a writable ledger handle from the ledger's own worker thread, which until now were queued back onto that same thread.Motivation
LedgerHandleruns its work on oneOrderedExecutorthread per ledger. When the caller is already on that thread, going throughexecute()costs a queue round trip on the same thread and delays the request behind whatever else is queued. With #4881 an application can put its own per-ledger work on that same thread (Pulsar's managed ledger runs onchooseThread(mlName)), which makes this the common case for tailing reads issued from the managed-ledger thread.Adds already run
PendingAddOp.initiate()inline on the caller's thread and reads on read-only handles already bypass the executor ("avoids a context-switch to OSE thread"); reads on writable handles were the remaining same-thread hop.A blanket shortcut inside
execute()would change the executor's contract, so the shortcut is opt-in: a task submitted from the executor's own thread runs before the tasks already queued and nested inside the submitting task, which is only acceptable where the caller knows it. This follows the pattern ofOrderedGenericCallback(inline when already on the thread selected by the key) and Netty'sinEventLoop().Changes
ThreadBoundExecutorinterface (isCurrentThread(),executeOrRun(Runnable)), implemented bySingleThreadExecutorand by the decoratorOrderedExecutorwraps its threads in when task tracing or MDC preservation is enabled (the decorator applies the timing and MDC wrappers to inline runs as well).SingleThreadExecutor.isCurrentThread()andexecuteOrRun(Runnable): runs the task inline when called from the executor's thread, otherwise delegates toexecute; likeexecute, it rejects tasks once the executor is shut down. Inline runs go through the samesafeRunTaskas queued tasks, so failures are logged and counted the same way; the queued path (runQueuedTask) only adds the pending-count decrement that pairs with the enqueue-time increment of bounded queues. Submitted and completed counters include inline runs, sogetQueuedTasksCount()stays consistent.LedgerHandle.executoris now typed asThreadBoundExecutor(cast at construction): the client's main worker pool is anOrderedExecutor, whose threads implement the interface whether or not they are decorated (OrderedScheduleroverrides the per-thread executor with a scheduled-executor wrapper, sochooseThreaditself cannot be declared to return the interface). The two read submission sites for writable handles, inasyncReadEntriesInternalandreadEntriesInternalAsync, callexecutor.executeOrRun(op).MockClientContextandMockBookKeeperTestCasehanded the client anOrderedScheduleras its main worker pool; they now provide anOrderedExecutor, with the mock bookie client dispatching on that same pool (the scheduler stays for timers). That also makes the mock bookie callbacks and the handle share one thread per ledger, as in production.OrderedExecutor.chooseThread(...)can useisCurrentThread()/executeOrRunfor their own re-dispatches; the Pulsar follow-up isOpAddEntry.addComplete, which re-enqueues onto the managed-ledger thread it is already on once Pin a ledger's callbacks to a caller-chosen worker thread via withOrderingKey #4881 is in use.Verification
TestOrderedExecutorDecorators: with task tracing or MDC preservation enabled, the pool's threads areThreadBoundExecutors and run inline from their own thread.TestSingleThreadExecutor:isCurrentThreadfrom both sides;executeOrRunfrom the executor thread runs before returning and ahead of an already queued task, with consistent counters; from another thread it is queued and runs on the executor thread; an inline failure is isolated from the submitting task and counted as failed.LedgerHandleInlineReadTest(mock bookies): a read issued on the ledger thread reaches the bookie client beforereadAsync/asyncReadEntriesreturn, on that same thread, with plain and with decorated (task tracing) worker threads; a read from another thread is still queued onto the ledger thread.TestOrderedExecutor,BookieReadWriteTest,TestSpeculativeRead,TestSpeculativeBatchRead,TestBatchedRead,TestReadLastConfirmedAndEntry,TestReadLastConfirmedLongPoll,BookKeeperTest, and the suites built on the changed fixtures (HandleFailuresTest,LedgerClose2Test,LedgerRecovery2Test,LedgerRecoveryTest,DeferredSyncTest,TestMaxEnsembleChangeNum,BookKeeperBuildersTest,BookKeeperBuildersOpenLedgerTest,LoggerContextTest,BookKeeperApiTest,MockBookKeeperTest,TestLedgerFragmentReplicationWithMock),MdcContextTest, and the replication suites that failed in the first CI run (AuditorPeriodicCheckTest,BookieAutoRecoveryTest,BookieDecommissionTest): all green, excepttestSequenceReadLocalEnsemblein the two speculative-read classes, which fails on the development machine on any branch because its hostname does not resolve (the local placement policy then derives a loopback bookie address that the client-side configuration rejects while constructing the test client).checkstyle:checkandspotbugs:checkpass on bookkeeper-common and bookkeeper-server.