Repository navigation
[Store] Run master RPC handlers on worker threads so a slow request cannot delay Ping - #4601
Conversation
4064896 to
c53a52d
Compare
Icedcoco
left a comment
There was a problem hiding this comment.
Thanks for adding this worker pool; moving the handlers off the RPC I/O threads looks useful for keeping Ping responsive when a handler is slow. I had two questions while reviewing the overload and configuration paths.
It looks like each request is submitted with coro_io::post, which queues work on the Asio thread pool. The worker count limits how many handlers run at once, but I couldn’t find a bound on how many requests can wait in the queue. If requests arrive faster than the workers can process them for a sustained period, queued tasks and the request state they retain could accumulate. Would it be worth adding a queue limit or another form of backpressure, with a clear overload response? I may be overlooking an existing admission control, so I’d be glad to hear how you expect this to behave under sustained load.
I also noticed that the CLI flag is a signed int32_t, while handler_worker_thread_num is stored as an unsigned value. A setting such as --handler_worker_thread_num=-1 therefore converts to a very large number. Asio rejects thread counts that large, and this construction path doesn’t appear to catch the exception, so the master would fail during startup. Could we validate that the value is non-negative and report a clear configuration error? The current 0 default behavior could remain unchanged.
|
Thanks for the careful review! Unbounded queue. You're right, and the PR body lists it as a known limitation, with the bound and metrics planned for a follow-up. One detail shapes what that bound can look like: coro_rpc starts a coroutine handler detached ( Negative thread count. Good catch, confirmed. Fixed in 2894006: a gflags validator now rejects negative values at startup with a clear error, and |
2894006 to
2cfd07d
Compare
Icedcoco
left a comment
There was a problem hiding this comment.
LGTM pending CI.
One consideration for a follow-up: the worker pool’s queue is unbounded, so sustained overload could lead to growing memory usage. It may be worth documenting the expected overload behavior or tracking admission control separately.
…annot delay Ping coro_rpc runs a plain handler inline on the io thread that owns its connection, so one slow handler stops every other connection on that thread from being read, including the Ping of healthy clients. RpcWorkerPool registers a method as a coroutine handler under the same route key; the coroutine posts the call to a thread pool, so the io thread goes back to reading. RegisterRpcService puts every handler except Ping and ServiceReady on the pool. The master gets a new handler_worker_thread_num flag and config key; 0 starts as many worker threads as rpc_thread_num. HA mode still runs handlers inline. Part of kvcache-ai#4181.
The flag is a signed int32 but the config stores it unsigned, so -1 became UINT32_MAX, skipped the 0 fallback, and asio::thread_pool threw std::out_of_range outside any try block, killing the master. A gflags validator now rejects negative values with a clear error; 0 still means as many workers as rpc_thread_num.
2cfd07d to
84d5b30
Compare
The worker pool was only wired into the non-HA master, so an HA leader still ran every handler inline on its io threads, and a slow request could still delay the Ping of healthy clients. The threading model should not depend on whether HA is enabled. MasterServiceSupervisor now starts an RpcWorkerPool for each leader term and registers the term's handlers on it. handler_worker_thread_num is carried into MasterServiceSupervisorConfig; 0 again means as many workers as rpc_thread_num. The pool holds a reference to the term's service, so every place that releases the service drains the pool first. When the term's server stops, the pool finishes the calls it still holds before the OpLog writer stops and leadership is released. Inline handlers gave the same guarantee, since they ran on the io threads that stopping the server waits for.
Since kvcache-ai#4601 the master's RPC handlers run on the RPC worker pool, and coro_rpc_server::stop() does not wait for the handlers an io thread has already handed to it. The handoff shutdown therefore destroys the pool after it stops the server and before it quiesces the service, so that a late handler cannot change the index while the snapshot is written. Move that step from mooncake_master's main() into StopRpcServerAndDrainHandlers (rpc_service) and test it on a real MasterService (HandoffEndToEndTest.PooledHandlersFinishBeforeTheSnapshot): a pooled handler that is still running when the stop step begins removes a key one second later, and the snapshot must match the index the handler leaves. Without the pool drain, the snapshot is written first and still holds the key.
Since kvcache-ai#4601 the master's RPC handlers run on the RPC worker pool, and coro_rpc_server::stop() does not wait for the handlers an io thread has already handed to it. The handoff shutdown therefore destroys the pool after it stops the server and before it quiesces the service, so that a late handler cannot change the index while the snapshot is written. Move that step from mooncake_master's main() into StopRpcServerAndDrainHandlers (rpc_service) and test it on a real MasterService (HandoffEndToEndTest.PooledHandlersFinishBeforeTheSnapshot): a pooled handler that is still running when the stop step begins removes a key one second later, and the snapshot must match the index the handler leaves. Without the pool drain, the snapshot is written first and still holds the key.
Since kvcache-ai#4601 the master's RPC handlers run on the RPC worker pool, and coro_rpc_server::stop() does not wait for the handlers an io thread has already handed to it. The handoff shutdown therefore destroys the pool after it stops the server and before it quiesces the service, so that a late handler cannot change the index while the snapshot is written. Move that step from mooncake_master's main() into StopRpcServerAndDrainHandlers (rpc_service) and test it on a real MasterService (HandoffEndToEndTest.PooledHandlersFinishBeforeTheSnapshot): a pooled handler that is still running when the stop step begins removes a key one second later, and the snapshot must match the index the handler leaves. Without the pool drain, the snapshot is written first and still holds the key.
Since kvcache-ai#4601 the master's RPC handlers run on the RPC worker pool, and coro_rpc_server::stop() does not wait for the handlers an io thread has already handed to it. The handoff shutdown therefore destroys the pool after it stops the server and before it quiesces the service, so that a late handler cannot change the index while the snapshot is written. Move that step from mooncake_master's main() into StopRpcServerAndDrainHandlers (rpc_service) and test it on a real MasterService (HandoffEndToEndTest.PooledHandlersFinishBeforeTheSnapshot): a pooled handler that is still running when the stop step begins removes a key one second later, and the snapshot must match the index the handler leaves. Without the pool drain, the snapshot is written first and still holds the key.
Since kvcache-ai#4601 the master's RPC handlers run on the RPC worker pool, and coro_rpc_server::stop() does not wait for the handlers an io thread has already handed to it. The handoff shutdown therefore destroys the pool after it stops the server and before it quiesces the service, so that a late handler cannot change the index while the snapshot is written. Move that step from mooncake_master's main() into StopRpcServerAndDrainHandlers (rpc_service) and test it on a real MasterService (HandoffEndToEndTest.PooledHandlersFinishBeforeTheSnapshot): a pooled handler that is still running when the stop step begins removes a key one second later, and the snapshot must match the index the handler leaves. Without the pool drain, the snapshot is written first and still holds the key.
Description
Part of RFC #4181 (thread dimension). This takes the "run every non-
Pinghandler on a worker pool" alternative from §5 of the RFC instead of a separate heartbeat server (#4183).Problem. coro_rpc binds each connection to one io thread for its whole lifetime and runs a plain (non-coroutine) handler inline on that thread. A slow handler, such as
GetReplicaListByRegexover millions of keys or anUnmountSegmentthat runsClearInvalidHandles, therefore stops its io thread from reading any other connection bound to it. That includes thePingof healthy clients, which the master then moves toSUSPECTEDand eventuallyOFFLINE.Change.
RpcWorkerPool(rpc_worker_pool.h). It registers a method as a coroutine handler under the same route key that coro_rpc derives from the method's name (coro_rpc::func_id<Method>()), so the wire format and the client side are unchanged. The coroutine only posts the call to anasio::thread_pooland awaits it, so the io thread goes back to reading right away. The pool is generic over the service type and does not depend onWrappedMasterService.RegisterRpcServicetakes an optionalRpcWorkerPool*. With a pool, every handler exceptPingandServiceReadyruns on its worker threads; those two stay on the io threads, which the pool now keeps free. Without one (embedded master, tests), every handler runs inline as before.handler_worker_thread_num.0(the default) starts as many worker threads asrpc_thread_num, so the master runs data-plane handlers on worker threads in both HA and non-HA mode. The help text and the deployment guide now describerpc_thread_numas the io thread count.MasterServiceSupervisorstarts a pool for each leader term. When the term's server stops, the pool finishes the calls it holds before the OpLog writer stops and leadership is released, matching what inline handlers guaranteed; it drains before the term's service is destroyed.Compared with #4183, there is no second port and no client-side discovery. The cost is one thread hop per RPC (see the numbers below).
Behaviour changes worth reviewing
MasterClientpipelines on a connection only once a process has more than 160 requests in flight to the master (the yltclient_poolreuse limit). Below that, concurrent calls already use separate connections, which could be bound to different io threads and run concurrently, so handlers never had an ordering guarantee between concurrent calls.rpc_thread_numto2 × rpc_thread_num.Known limitation: the worker queue is unbounded and not observable. On main, a busy io thread stops reading its connections, so overload shows up as requests waiting in socket buffers and as late
Pings. With the pool, the io threads keep accepting requests and queue them for the workers, so:MasterClientcall blocks its caller until the response arrives, so the queue depth stays bounded by the number of calls the clients have in flight; it does not grow on its own.Pingstays fast by design, so a saturated master can be told apart from a healthy one only by data-plane latency.A bounded queue that rejects excess requests (for example with a busy error the client can retry), and metrics for queue depth and queue wait, are left for a follow-up.
Out of scope / follow-ups
Pingstill waits onclient_mutex_behind exclusive holders such asReMountSegment; that is [Store] Centralize client session lifecycle and resource ownership #4180.Benchmark. A throwaway benchmark (not included in this PR) started an in-process master with 2 io threads and N keys, then ran 4 clients calling
GetReplicaListByRegexback to back with a pattern that matches nothing (a full key walk with a tiny response), 2 clients callingGetReplicaListback to back, and 1 client sending aPingevery 20 ms. Connections were opened one by one so that thePingconnection always shared an io thread with slow clients. 3 runs per row.Pingp50PingmaxGetReplicaListp50On an idle master the hop costs about 4 µs per call (p50 of a point RPC 10 µs → 14 µs). With the pool saturated,
Pingis still served promptly; data-plane calls wait in the worker queue instead.Module
mooncake-transfer-engine)mooncake-store)mooncake-conductor)mooncake-reshard)mooncake-ep)mooncake-pg)mooncake-integration)mooncake-p2p-store)mooncake-wheel)mooncake-common)mooncake-rl)Type of Change
How Has This Been Tested?
Test commands:
Test results:
Unit tests pass
Integration tests pass (if applicable)
Manual testing done (describe below)
rpc_worker_pool_test(new, 2 cases): pooled handlers, including a zero-argument one, are reachable under their method names; and with a single io thread, a pooled handler that never returns leaves the io thread free to accept a new connection and answer an inlinePing.batch_replica_clear_tenant_rpc_test1/1 andmaster_service_config_test19/19 pass; a new case checks thathandler_worker_thread_numreachesMasterServiceSupervisorConfig.high_availability_testpasses locally (5 passed, 2 skipped), but this build has no etcd backend, so the case that runs the supervisor through a leader term (HaWithoutOplogRestoresEmptyContextAndServes) only runs in CI, where etcd is available.mooncake_masterstarted withhandler_worker_thread_numresolved torpc_thread_num(16/16 in the log), servedmaster_benchBatchPut and BatchGet without errors, and exited cleanly on SIGTERM.The changed translation units and the new test pass
g++ 11.4 -fsyntax-only(Ubuntu 22.04, the CI compiler).The docs build with Sphinx, and the new flag row renders in the deployment guide.
Checklist
./scripts/code_format.shAI Assistance Disclosure
Claude Code helped implement the worker pool, write the unit test and the throwaway benchmark, and draft this description. I have reviewed every changed line and can defend the change end to end.