KAFKA-20650: Share ClientQuotaCallback across broker and controller in combined mode#22792
Open
vismaytiwari wants to merge 2 commits into
Open
KAFKA-20650: Share ClientQuotaCallback across broker and controller in combined mode#22792vismaytiwari wants to merge 2 commits into
vismaytiwari wants to merge 2 commits into
Conversation
The deserialize(String, Headers, ByteBuffer) Javadoc stated only what callers and implementations cannot assume about the buffer (position, limit, capacity), leaving the readable region and side-effect expectations unspecified. This clarifies that the serialized bytes are the buffer's remaining() bytes and that implementations should not modify the buffer's position/limit/mark, since the caller may read the buffer again afterward (for example, to determine the serialized value size). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…n combined mode In combined (KRaft) mode, BrokerServer and ControllerServer each called QuotaFactory.instantiate(), which created a separate ClientQuotaCallback instance via reflection for each role. A custom callback was therefore instantiated twice with divergent state in a single JVM - a regression from the pre-KRaft single-instance behavior. Create the callback once in SharedServer (already shared by the broker and controller roles) and pass it to a new QuotaFactory.instantiate() overload so both roles use the same instance. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
github-actions
Bot
added
triage
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.
JIRA: https://issues.apache.org/jira/browse/KAFKA-20650
Problem
In combined (KRaft) mode,
BrokerServerandControllerServereach callQuotaFactory.instantiate(), which creates a separateClientQuotaCallbackvia reflection per role. A custom callback is therefore instantiated twice with divergent state in one JVM — a regression from the pre-KRaft single-instance behavior.Approach
Create the callback once in
SharedServer(which the broker and controller already share formetrics,raftManager, etc.) and pass it into a newQuotaFactory.instantiate(...)overload that accepts a pre-builtOptional<Plugin<ClientQuotaCallback>>, so both roles use the same instance. Separate-mode broker-only / controller-only behavior is unchanged (one role perSharedServer).Open questions for reviewers (this is why I'm raising it as a discussion)
QuotaManagers.shutdown(), so in combined mode it is closed twice (benign viaUtils.closeQuietly, but not ideal). I can instead track ownership so onlySharedServercloses it, or haveSharedServerown the close andQuotaManagersskip it — happy to implement whichever you prefer. I kept this PR minimal to align on the approach first rather than churn theQuotaManagersrecord + its test call sites.sharedServerConfigwith thebrokerrole tag. In combined mode the broker/controller configs share the same props, but let me know if you'd prefer a different config or a combined role tag.Testing / honesty
I couldn't run a local build for this (no JDK available in my environment), so I've relied on careful manual review and am leaning on CI here — apologies if something needs a fixup; I'll iterate quickly. Guidance on the two questions above would let me finish it cleanly.
AI disclosure: drafted with Claude (Claude Code) and reviewed; the commit carries the
Co-Authored-Bytrailer per the contributing guide.