Park pulls awaiting registration instead of holding a server thread - #1026
Open
jndevanshu wants to merge 4 commits into
Open
jndevanshu wants to merge 4 commits into
jndevanshu wants to merge 4 commits into
Conversation
A producer that receives a PullStream for a read it has not registered yet waits up to kPullRegistrationGrace (5 s) for NotifyForRead. That wait ran on the control server's handler thread, and the gRPC handler ignored the caller's deadline and cancellation, so a burst of early, expired or bogus pulls from one consumer tied up one thread each for the full grace and starved pulls from every other consumer (issue #888). The gRPC PullStream is now a callback-API reactor. A pull whose read is not registered is parked as an entry with no thread behind it, and is answered: - by NotifyForRead as soon as the read is registered; - with a rejection when the grace or the consumer's deadline passes, whichever is first, by a single expiry thread; - with a rejection as soon as the consumer cancels the call; - with "stopping" on shutdown, before the server is stopped, since a callback server's shutdown waits for every outstanding call. Parked pulls are capped at kMaxPullWaitersPerPeer (64) per peer transport address and kMaxPullWaiters (4096) in total. Over a cap a pull is rejected at once, so one consumer cannot use up the room every other consumer needs. ControlPlaneHandler gains OnPullStreamAsync/CancelPullStream, whose defaults call OnPullStream inline. The TCP backend, which is being retired, keeps its blocking path through a wrapper over the same parking logic.
GrpcPuller used GrpcControlPlaneBackend::SendPullRequestAsync, which is not on main. It now drives the generated stub's callback API directly, with a ClientContext per call, so tests can also cancel pulls explicitly. It no longer builds a StatusOr from an OK status, which aborted. The producer gives up on a parked pull at the consumer's deadline, so its rejection and the consumer's own DEADLINE_EXCEEDED race. The deadline test accepts either, and still requires the pull to be released well before the registration grace.
The pull rejections were only tested over TCP, where the backend refuses an oversized pull from its header before the handler sees it, so the handler's own block-count check was never reached. Validation of a pull that was parked and then resolved by NotifyForRead was not tested at all. New gRPC tests cover the oversized pull (rejected without parking), the unregistered, duplicate and empty block lists against a registered read and against a parked one on registration, a registration that has already expired (before and after the pull), and a duplicate pull.
Main's fault-injection hooks (62e98ea) were placed in the synchronous gRPC PullStream and the blocking registration wait, which this branch replaced. They keep their meaning in the new code: - grpc_control_plane.pull.reply is applied in PullStreamReactor::Respond, replacing a successful handler answer with the injected error, on every path that answers a pull. - kv_cache_manager.pull.register_wait delays BeginPullStream after the block-count check, ahead of the registration lookup. The pull.accepted and pull.spawn hooks merged cleanly into LaunchPull, inside its try, so an injected throw still becomes a rejection.
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
1 participant
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.
When a producer receives a PullStream for a read it has not registered yet, it waits up to kPullRegistrationGrace (5 s) for NotifyForRead. That wait ran on the control server's handler thread, and the gRPC handler ignored the caller's deadline and cancellation. A burst of early, expired or bogus pulls from one consumer therefore held one server thread each for the full grace. That starved pulls from every other consumer, and they are the ones the producer could have answered right away.