KAFKA-20828: Centralize client throttling version logic - #22989
KAFKA-20828: Centralize client throttling version logic#22989arnabnandy7 wants to merge 2 commits into
Conversation
Signed-off-by: Arnab Nandy <arnab_nandy7@yahoo.com>
junrao
left a comment
junrao
left a comment
There was a problem hiding this comment.
@arnabnandy7 : Thanks for the PR. Left a comment.
| public static final int DEFAULT_THROTTLE_TIME = 0; | ||
|
|
||
| private static final Map<ApiKeys, Short> POST_KIP_219_VERSION = Map.ofEntries( | ||
| Map.entry(ApiKeys.PRODUCE, (short) 6), |
There was a problem hiding this comment.
Hmm, is this approach better? If we ever deprecate an old version of a request, this introduces another place that needs to be updated.
There was a problem hiding this comment.
@junrao my understanding is that these entries represent the fixed historical version where client-side throttling began, rather than the currently supported or non-deprecated version range.
Deprecating an older version should therefore not require updating this map. An entry may become redundant as the minimum supported version advances, but it would remain behaviorally correct.
That said, if you prefer avoiding the duplicated protocol-version metadata entirely, I’m happy to retain the version-specific overrides in the response classes.
There was a problem hiding this comment.
could you add comment for the POST_KIP_219_VERSION? For example:
These values represent immutable historical version boundaries for KIP-219 client-side throttling. They do not change when older API versions are deprecated.
is this approach better?
I love this approach because it is more simple and readable.
There was a problem hiding this comment.
sure @chia7712, documenting it better as suggested.
There was a problem hiding this comment.
Added the comment as well.
Signed-off-by: Arnab Nandy <arnab_nandy7@yahoo.com>
3 participants
Description
Follow-up to KAFKA-20828 and PR #22908.
This change centralizes the historical KIP-219 client-throttling version boundaries in
AbstractResponse.shouldClientThrottle().For APIs that existed before KIP-219, the centralized mapping determines the first version that supports client-side throttling. For newer APIs, the behavior continues to be derived from the presence of
throttle_time_msin the response schema.This preserves the existing version-specific behavior while removing all 37
shouldClientThrottle()overrides from individual response classes. Future throttling behavior can now be maintained in one place instead of across many subclasses.Testing
The existing comprehensive regression test checks
shouldClientThrottle()for every supported version of every API key. This verifies both:The following checks passed:
clients:test --tests org.apache.kafka.common.requests.RequestResponseTestReviewers: Jun Rao junrao@gmail.com, Chia-Ping Tsai chia7712@gmail.com