Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

extend throttling metric scope #2533

Merged
merged 5 commits into from
Jul 29, 2023

Conversation

hindessm
Copy link
Collaborator

This is PR is refactoring the current throttling metric support with a view to implementing throttling.

Currently, there were three different throttle time field implementations:

  1. a ThrottleTimeMs int32 field - for example, on MetadataResponse,
  2. a ThrottleTime int32 field - for example, on LeaveGroupResponse, and
  3. a ThrottleTime time.Duration field - for example, on ProduceResponse.

It would be nice to make these more consistent but, since they are public fields, that would need to be a v2 change.
Therefore, in order to make progress now, this PR implements a common, private throttleTime() time.Duration method on all of the responses covered by the above three field types. The updateThrottleMetric implementation is then refactored to apply more generally to responses that have the new common method (via a trivial new interface throttleSupport).

This should make it possible to implement throttling support - i.e. delaying future requests in accordance with KIP-219.

Two typos are also fixed - one related and one randomly spotted while in broker.go.

Strictly-speaking this is a field name typo but as it's public
fixing that will have to wait until v2.

Signed-off-by: Mark Hindess <mark.hindess@gmail.com>
Signed-off-by: Mark Hindess <mark.hindess@gmail.com>
Signed-off-by: Mark Hindess <mark.hindess@gmail.com>
This should be sufficient to allow throttling support to
be added.

Signed-off-by: Mark Hindess <mark.hindess@gmail.com>
Signed-off-by: Mark Hindess <mark.hindess@gmail.com>
broker.go Dismissed Show dismissed Hide dismissed
Copy link
Collaborator

@dnwe dnwe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@dnwe
Copy link
Collaborator

dnwe commented Jul 29, 2023

Raised #2534 for flake and rerunning

@dnwe dnwe merged commit f07b129 into IBM:main Jul 29, 2023
13 checks passed
@dnwe dnwe added the fix label Jul 29, 2023
@hindessm hindessm deleted the mrh/extend-throttling-metric-scope branch July 29, 2023 18:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants