Process kafka requests in a separate scheduling group #24973
Conversation
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]>
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_sg` Signed-off-by: Michał Maślanka <[email protected]>
Signed-off-by: Michał Maślanka <[email protected]>
CI test resultstest results on build#61350
|
mmaslankaprv
requested review from
dotnwat,
travisdowns,
StephanDollberg and
ballard26
| @@ -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).
| coordinator_ntp_mapper& server::coordinator_mapper() { | ||
| return _group_router.local().coordinator_mapper().local(); | ||
| } | ||
|
|
||
| ss::scheduling_group server::get_request_scheduling_group(api_key key) const { | ||
| if (key == produce_request::api_type::key) { |
There was a problem hiding this comment.
For this kind of stuff usually we want to add a handler method I think, rather than having the kafka server know anything about the different keys. E.g., a get_request_sg() method on the generic handler which returns the group (or none/default) to use?
Is the problem that the handlers shouldn't access config?
There was a problem hiding this comment.
good point, i will change that
| /** | ||
| * 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!
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