Skip to content

KAFKA-2209 - Change quotas dynamically using DynamicConfigManager#298

Closed
auradkar wants to merge 10 commits intoapache:trunkfrom
auradkar:K-2209
Closed

KAFKA-2209 - Change quotas dynamically using DynamicConfigManager#298
auradkar wants to merge 10 commits intoapache:trunkfrom
auradkar:K-2209

Conversation

@auradkar
Copy link
Copy Markdown
Contributor

Changes in this patch are:

  1. ClientIdConfigHandler now passes through the config changes to the quota manager.
  2. Removed static KafkaConfigs for quota overrides. These are no longer needed since we can override configs through ZooKeeper.
  3. Added testcases to verify that the config changes are propogated from ZK (written using AdminTools) to the actual Metric objects.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do you really just want to record a 0 value?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The idea was to change the quota and test that the throttle time is 0 for the new quota.
Recording a 0 doesn't change the underlying value. It is basically trying to verify that new quota is applied and doesn't throttle on the previously recorded value.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If the client is not throttled, the throttle-time should always be 0, right? Is there a need to for using EPS? Ditto below.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The assertEquals(expected, actual) method has been deprecated, so I used the assertEquals(expected, actual, delta) method. I can set the delta to zero in this case.

@junrao
Copy link
Copy Markdown
Contributor

junrao commented Oct 19, 2015

Also, in ClientQuotaManager.recordAndMaybeThrottle(), it seems that we shouldn't record throttleTimeMs, if it's 0. Otherwise, it will skew the computation of Avg().

@auradkar
Copy link
Copy Markdown
Contributor Author

I suppose we could interpret it 2 ways:

  1. The throttle time metric should indeed count unthrottled as 0 since it is a valid value. Basically, the metric exposes average throttle time per-request from a given clientId. For unthrottled client-ids, it will return 0.
  2. The metric exposes the throttle time when a client-id actually got throttled. I suppose this is more intuitive. I changed the Avg metric to return 0 by default (when there are no samples).

@junrao
Copy link
Copy Markdown
Contributor

junrao commented Oct 21, 2015

Thanks for the patch. LGTM.

Could you rebase? Also, is assertEquals(expected, actual) really deprecated? You are using that in testClientQuotaConfigChange() too.

@auradkar
Copy link
Copy Markdown
Contributor Author

Rebased and published a new patch.

And yes, assertEquals is deprecated for the versions that compares "double" or "float". The one in testClientQuotaConfigChange compares the Quota object directly so no "delta" parameter on the method.

@junrao
Copy link
Copy Markdown
Contributor

junrao commented Oct 21, 2015

Could you rebase again? Guozhang just pushed a big patch on the new consumer.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants