merlimat opened a new pull request, #4883:
URL: https://github.com/apache/bookkeeper/pull/4883
Descriptions of the changes in this PR:
Adds an opt-in inline execution path to `SingleThreadExecutor` and 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
`LedgerHandle` runs its work on one `OrderedExecutor` thread per ledger.
When the caller is already on that thread, going through `execute()` 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 on `chooseThread(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 of
`OrderedGenericCallback` (inline when already on the thread selected by the
key) and Netty's `inEventLoop()`.
### Changes
- `SingleThreadExecutor.isCurrentThread()` and `executeOrRun(Runnable)`:
runs the task inline when called from the executor's thread, otherwise
delegates to `execute`. Inline runs go through the same `safeRunTask` as 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, so `getQueuedTasksCount()` stays consistent.
- `LedgerHandle.executor` is now typed as `SingleThreadExecutor` (cast at
construction): the client's main worker pool is a plain `OrderedExecutor`,
whose threads are `SingleThreadExecutor` instances (`OrderedScheduler`
overrides the per-thread executor with a scheduled-executor wrapper, so
`chooseThread` itself cannot be declared to return the concrete type). The two
read submission sites for writable handles, in `asyncReadEntriesInternal` and
`readEntriesInternalAsync`, call `executor.executeOrRun(op)`.
- Test fixtures: `MockClientContext` and `MockBookKeeperTestCase` handed the
client an `OrderedScheduler` as its main worker pool; they now provide an
`OrderedExecutor`, 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.
- Applications holding a worker thread from
`OrderedExecutor.chooseThread(...)` can use `isCurrentThread()` /
`executeOrRun` for their own re-dispatches; the Pulsar follow-up is
`OpAddEntry.addComplete`, which re-enqueues onto the managed-ledger thread it
is already on once #4881 is in use.
### Verification
- `TestSingleThreadExecutor`: `isCurrentThread` from both sides;
`executeOrRun` from 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.
- New `LedgerHandleInlineReadTest` (mock bookies): a read issued on the
ledger thread reaches the bookie client before `readAsync` / `asyncReadEntries`
return, on that same thread; a read from another thread is still queued onto
the ledger thread.
- Existing tests: `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`): all green, except
`testSequenceReadLocalEnsemble` in 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:check` and `spotbugs:check` pass on bookkeeper-common and
bookkeeper-server.
--
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]
