Skip to content

KAFKA-19315: Move ControllerMutationQuotaManager to server module #19807

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

Open
wants to merge 46 commits into
base: trunk
Choose a base branch
from

Conversation

YutaLin
Copy link
Contributor

@YutaLin YutaLin commented May 25, 2025

Migrate ControllerMutationQuotaManager to Java implementation and move
to server module, including ClientQuotaManager and associated files.

Reviewers: PoAn Yang [email protected],
Ken Huang [email protected], TengYao
Chi [email protected], Chia-Ping
Tsai [email protected]

@github-actions github-actions bot added triage PRs from the community core Kafka Broker performance labels May 25, 2025
YutaLin added 6 commits May 25, 2025 10:46
@YutaLin YutaLin changed the title KAFK1-19315: Move ControllerMutationQuotaManager to server module KAFKA-19315: Move ControllerMutationQuotaManager to server module May 26, 2025
Copy link
Collaborator

@m1a2st m1a2st left a comment

Choose a reason for hiding this comment

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

Thanks @YutaLin for this patch, left some comments

YutaLin added 2 commits May 26, 2025 21:35
@github-actions github-actions bot removed the triage PRs from the community label May 27, 2025
Copy link
Contributor

@frankvicky frankvicky left a comment

Choose a reason for hiding this comment

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

@YutaLin: Please fix the conflicts

Copy link
Collaborator

@m1a2st m1a2st left a comment

Choose a reason for hiding this comment

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

Thanks @YutaLin for update, few more comments

YutaLin added 9 commits May 28, 2025 07:44
@YutaLin
Copy link
Contributor Author

YutaLin commented May 29, 2025

We will continue tracking the failing test PlaintextConsumerCommitTest > testCommitAsyncCompletedBeforeConsumerCloses [1] Type=Raft-Isolated, which is recorded in https://issues.apache.org/jira/browse/KAFKA-19352.

Copy link
Member

@chia7712 chia7712 left a comment

Choose a reason for hiding this comment

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

@YutaLin thanks for this patch. a couple of comments are left

protected final Metrics metrics;
private final QuotaType quotaType;
protected final Time time;
private final Optional<Plugin<ClientQuotaCallback>> clientQuotaCallbackPlugin;
Copy link
Member

Choose a reason for hiding this comment

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

we don't need to keep it as member of ClientQuotaManager

Copy link
Member

Choose a reason for hiding this comment

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

I assume you will remove this variable, but you don't. Could you please share the reason with me?

YutaLin added 11 commits May 29, 2025 15:44
Copy link
Member

@chia7712 chia7712 left a comment

Choose a reason for hiding this comment

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

@YutaLin thanks for this patch. a couple of comments remain


public class ClientQuotaManager {

static final class QuotaTypes {
Copy link
Member

Choose a reason for hiding this comment

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

Do we really need a struct class?

}

private void start() {
throttledChannelReaper.start();
Copy link
Member

Choose a reason for hiding this comment

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

this method can be inlined to constructor

this.quotaTypesEnabled = clientQuotaCallbackPlugin.isPresent() ?
QuotaTypes.CUSTOM_QUOTAS : QuotaTypes.NO_QUOTAS;

this.delayQueueSensor = metrics.sensor(quotaType.toString() + "-delayQueue");
Copy link
Member

Choose a reason for hiding this comment

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

toString is redunant

int throttleTimeMs
) {
if (throttleTimeMs > 0) {
ClientSensors clientSensors = getOrCreateQuotaSensors(session, clientId);
Copy link
Member

Choose a reason for hiding this comment

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

it would be cool if we can leverage var to cleanup the type declaration.

            var clientSensors = getOrCreateQuotaSensors(session, clientId);
            clientSensors.throttleTimeSensor().record(throttleTimeMs);
            var throttledChannel = new ThrottledChannel(time, throttleTimeMs, throttleCallback);

* Recording any larger value will always be throttled, even if no other values were recorded in the quota window.
* This is used for deciding the maximum bytes that can be fetched at once
*/
public double getMaxValueInQuotaWindow(Session session, String clientId) {
Copy link
Member

Choose a reason for hiding this comment

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

could you please remove the prefix get

* This is used for deciding the maximum bytes that can be fetched at once
*/
public double getMaxValueInQuotaWindow(Session session, String clientId) {
if (quotasEnabled()) {
Copy link
Member

Choose a reason for hiding this comment

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

        if (!quotasEnabled()) return Double.MAX_VALUE;
        var clientSensors = getOrCreateQuotaSensors(session, clientId);
        var limit = quotaCallback.quotaLimit(clientQuotaType, clientSensors.metricTags());
        if (limit != null) return limit * (config.numQuotaSamples - 1) * config.quotaWindowSizeSeconds;
        return Double.MAX_VALUE;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants