KAFKA-6576: Configurable Quota Management (KIP-257) by rajinisivaram · Pull Request #4699 · apache/kafka

rajinisivaram · 2018-03-13T09:40:29Z

Enable quota calculation to be customized using a configurable callback. See KIP-257 for details.

Committer Checklist (excluded from commit message)

Verify design and implementation
Verify test coverage and CI build status
Verify documentation (including upgrade notes)

rajinisivaram · 2018-04-03T14:54:08Z

retest this please

junrao

@rajinisivaram : Thanks for the patch. Left a few comments below.

junrao · 2018-04-03T00:58:53Z

Typo "is takes ".

junrao · 2018-04-04T20:54:53Z

Since we already have quotaLimit(), could quota() just return the metric tags? The caller can then use the metric tags to obtain the quota limit. This way, there is only one way to get the quota limit.

@junrao We need to go through the quota configs at different levels to determine the metric tags. quotaLimit also does something similar. So it is cheaper to return the limit with the tags, and since this is used for every request, it may be worth returning limit and tags together. With two calls, we also need to handle changes between the two invocations. Alternatively, we could do something different for default callback handler alone since custom handlers are less likely to have several levels. What do you think?

@rajinisivaram : Hmm, my understanding is that quota() only needs to be called when the quota metric doesn't already exist. So, typically, quota() is only called on the first request from a given client and not on every request.

@junrao At the moment, we call quota() on every request since need to work out which metric tags to use and to do that we need to know which level of configs are defined. But, you are right, after the first request, we already know the limit since the metric exists, so we don't need to call two methods every time. So agree, we can change the first method to just return metric tags. Will update the PR, thanks.

@junrao I have changed the first method to quotaMetricTags to obtain just the tags and updated the code to use quotaLimit to get the limit in all cases. Thanks.

junrao · 2018-04-04T20:58:00Z

The comment in line 472 probably needs to be adjusted.

junrao · 2018-04-04T20:59:54Z

Ideally, the callback shouldn't access internal objects such as allMetrics. So perhaps the logic under singleUpdate should be handled in the same place where multiple updates are handled?

junrao · 2018-04-04T21:49:46Z

This is not introduced in this patch. But I am wondering why we need to pass in a separate MetricConfig to throttleTime() since clientMetric contains MetricConfig already.

Not sure, I changed it and tests still passed, so I guess it is to remove it.

junrao · 2018-04-04T22:42:23Z

.toMap is not needed.

junrao · 2018-04-04T22:58:57Z

metric.value is deprecated. Could we use the new api? There are a few other places like that.

junrao · 2018-04-04T23:23:06Z

Could we add some doc of this class? For example, how is the naming convention of group for quota and topics.

junrao · 2018-04-04T23:56:23Z

Perhaps add a comment on what brokerId is used for.

junrao · 2018-04-05T00:11:30Z

"some.prop" is not a valid prop. So perhaps rename invalidProps to invalidReconfigurableProps ?

rajinisivaram · 2018-04-05T16:21:36Z

@junrao Thanks for the review. I have moved the public interface to clients and converted it to Java. I have made most of the updates from the review, left a question about the interface.

junrao

@rajinisivaram : Thanks for the updated patch. A few more comments below.

junrao · 2018-04-05T23:05:24Z

+     * @param quotaType Type of quota requested
+     * @return quota metric tags that indicate which other clients share this quota
+     */
+    Map<String, String> quotaMetricTags(KafkaPrincipal principal, String clientId, ClientQuotaType quotaType);


Do we need quotaType? It seems that it's unrelated to metric tags.

When multiple levels of quotas are used, the tags are dependent on which level config is chosen (e.g for {user, client} quota, both are included, for {user} quota, empty client-id is used). Since we allow configuring different levels of quota for Produce/Fetch/Request, they can potentially use different tags for the same client.

Apart from updateClusterMetadata, all other methods are processed by individual ClientQuotaManagers and they pass in the quota type. I have moved the quota type as the first parameter of all these methods to keep them consistent.

junrao · 2018-04-05T23:15:17Z

-        clientRateMetricName(clientQuotaEntity.sanitizedUser, clientQuotaEntity.clientId),
-        Some(getQuotaMetricConfig(clientQuotaEntity.quota)),
+        clientRateMetricName(metricTags),
+        Some(getQuotaMetricConfig(metricTags)),


This can be a bit tricky. If we call getQuotaMetricConfig() here, we will be paying the cost of calling ClientQuotaCallback.quotaLimit() on every request. We probably want to pass in a method and only call it when needed.

I was thinking that the by-name parameter does lazy evaluation and hence the method doesn't get invoked on every request. I could be wrong, but I added a test and it seems to be true. What do you think?

junrao · 2018-04-05T23:34:37Z

-    else QuotaTypes.ClientIdQuotaEnabled
+  private val clientQuotaType = quotaTypeToClientQuotaType(quotaType)
+  @volatile private var quotaTypesEnabled = clientQuotaCallback match {
+    case Some(_) => QuotaTypes.CustomQuotas


Is it useful to have CustomQuotas since it seems it's never used?

It is used to avoid optimizing updates, the other values are used to check if only a single-level of quota is used. Have added a comment.

junrao · 2018-04-06T00:31:07Z

+     * @param newValue    The new quota value
+     * @return true if quotas have changed and metric configs may need to be updated
+     */
+    boolean updateQuota(ClientQuotaEntity quotaEntity, ClientQuotaType quotaType, double newValue);


Is there a case when updateQuota() can return false? Ditto for removeQuota().

I was using the return value with the default callback handler earlier, but now that it has been tidied up, we don't need it. Changed both updateQuota and removeQuota to void returns.

junrao · 2018-04-06T00:40:46Z

+    val origProps = TestUtils.createBrokerConfig(0, TestUtils.MockZkConnect, port = 8181)
+    val config = KafkaConfig(origProps)
+    val invalidReconfigurableProps = Set(KafkaConfig.LogCleanerThreadsProp, KafkaConfig.BrokerIdProp, "some.prop")
+    val validProps = Set(KafkaConfig.LogCleanerThreadsProp, KafkaConfig.LogCleanerDedupeBufferSizeProp, "some.prop")


We probably want to rename validProps to validReconfigurableProps for consistency?

rajinisivaram · 2018-04-06T18:55:35Z

@junrao Thanks for the review. I have updated the code and also made some changes to CustomQuotaCallbackTest to make it complete faster (the tests check only the metrics on the broker-side to avoid waiting for throttled requests to complete). Client-side metrics are verified by the other quota tests anyway.

I also added a new method to the callback interface to check if quotas need resetting. Since we now don't get quota limits from the callback handler on each request, we need to check if limits may have changed (e.g. with a callback that uses an external database to store quotas). Thoughts?

junrao

@rajinisivaram : Thanks for the updated patch. LGTM

rajinisivaram · 2018-04-06T21:48:50Z

@junrao Thanks for the reviews, merging to trunk.

Enable quota calculation to be customized using a configurable callback. See KIP-257 for details. Reviewers: Jun Rao <junrao@gmail.com>

rajinisivaram force-pushed the KAFKA-6576-configurable-quota branch from 47cff9b to 0c6b425 Compare March 16, 2018 19:08

rajinisivaram force-pushed the KAFKA-6576-configurable-quota branch 2 times, most recently from 65678dc to 0e12bc3 Compare March 29, 2018 11:22

junrao reviewed Apr 5, 2018

View reviewed changes

KAFKA-6576: Configurable Quota Management (KIP-257)

1590947

rajinisivaram force-pushed the KAFKA-6576-configurable-quota branch from 0e12bc3 to 1590947 Compare April 5, 2018 12:03

Address review comments

7dbd374

Address review comments

a857f03

junrao reviewed Apr 6, 2018

View reviewed changes

Address review comments

964362a

junrao approved these changes Apr 6, 2018

View reviewed changes

rajinisivaram merged commit 77ebd32 into apache:trunk Apr 6, 2018

Uh oh!

Conversation

rajinisivaram commented Mar 13, 2018

Committer Checklist (excluded from commit message)

Uh oh!

rajinisivaram commented Apr 3, 2018

Uh oh!

junrao left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rajinisivaram commented Apr 5, 2018

Uh oh!

junrao left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rajinisivaram commented Apr 6, 2018

Uh oh!

junrao left a comment

Choose a reason for hiding this comment

Uh oh!

rajinisivaram commented Apr 6, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants