Process kafka requests in a separate scheduling group #24973
Merged
Conversation
This file contains 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
CI test resultstest results on build#61350
test results on build#61495
|
mmaslankaprv
requested review from
dotnwat,
travisdowns,
StephanDollberg and
ballard26
StephanDollberg
previously approved these changes
Jan 30, 2025
| @@ -682,6 +684,10 @@ connection_context::dispatch_method_once(request_header hdr, size_t size) { | |||
| ? std::make_optional<ss::sstring>(*hdr.client_id) | |||
| : std::nullopt, | |||
| }; | |||
|
|
|||
| co_await ss::coroutine::switch_to( | |||
There was a problem hiding this comment.
@travisdowns related to the discussion from yesterday, does this even allocate the coroutine frame if await_ready returns true (because we are already in the right group - ignoring for a second whether we actually are here or not)? I thought not.
There was a problem hiding this comment.
Which coroutine do you refer to? In any case there shouldn't be a coroutine frame allocated per switch_to, whether the switch happens or not. What await_ready returning true does is avoids an unnecessary task switch (which is probably more expensive even than a coro frame in "micro" costs, and certainly in macro costs as it breaks out of the current executing request, etc).
travisdowns
reviewed
Jan 31, 2025
travisdowns
reviewed
Jan 31, 2025
| /** | ||
| * Scheduling group to process requests received via the REST API of | ||
| * admin server. | ||
| */ |
There was a problem hiding this comment.
Love to see these comments, thanks!
travisdowns
previously approved these changes
Jan 31, 2025
Added property allowing us to control if `kafka` scheduling group should be used to handle parsing and handling Kafka API requests. Signed-off-by: Michał Maślanka <[email protected]>
Signed-off-by: Michał Maślanka <[email protected]>
Signed-off-by: Michał Maślanka <[email protected]>
Added code that allows handler to control the scheduling group they are processed in. If a handler returns no override then the handler runs in a default `kafka` scheduling group. Signed-off-by: Michał Maślanka <[email protected]>
mmaslankaprv
dismissed stale reviews from travisdowns and StephanDollberg
via
b587cfc
mmaslankaprv
force-pushed
the
handler-sg
branch
from
44c7696 to
b587cfc
Compare
StephanDollberg
approved these changes
Feb 3, 2025
travisdowns
approved these changes
Feb 5, 2025
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.
Added code that allows using separate scheduling group for Kafka
requests processing. The scheduling group can be switched based on the
API type. By default processing is done in the group returned by
kafka::server::get_request_handler_sgBackports Required
Release Notes