Since there all the comments so far have been addressed, I will start vote for this KIP.
Regards, Rajini On Thu, Mar 15, 2018 at 6:37 PM, Rajini Sivaram <rajinisiva...@gmail.com> wrote: > Thanks, Jun. > > 11. updatePartitionMetadata() provides all partitions with their leaders > so that callbacks that scale down quotas based on fraction of partitions > hosted on each broker may compute the scaling factor. Callbacks that scale > up quotas based on the partition count hosted on each broker can simply > filter out the others. I have updated the Javadoc in the KIP. > > On Thu, Mar 15, 2018 at 5:24 PM, Jun Rao <j...@confluent.io> wrote: > >> Hi, Rajini, >> >> Thanks for the explanation. It makes sense. >> >> 11. We probably want to clarify in the interface that every time when >> updatePartitionMetadata() is called, the full list of partitions whose >> leader is on this broker will be passed in? >> >> Jun >> >> On Thu, Mar 15, 2018 at 4:42 AM, Rajini Sivaram <rajinisiva...@gmail.com> >> wrote: >> >> > Hi Jun, >> > >> > 12. Sorry, I had to revert the change that removed ` >> > ClientQuotaCallback.quotaLimit()`. We are allowing quota callbacks to >> use >> > custom metric tags. For each request, quota manager uses ` >> > ClientQuotaCallback.quota()` to map (user-principal, client-id) to the >> > metric tags that determine which clients share the quota. When quotas >> are >> > updated using `updateQuota` or `updatePartitionMetadata`, existing >> metrics >> > need to updated, but quota managers don't have a reverse mapping of >> metric >> > tags to (user-principal, client-id) for invoking`ClientQuotaCallback. >> > quota() >> > ` . Callbacks cannot return all updated metrics since they don't have >> > access to the metrics object and we don't want to require callbacks to >> > track all the entities for which metrics have been created (since they >> may >> > contain client-ids and hence need expiring). With the extra method, >> quota >> > manager traverses the metric list after `updateQuota` or ` >> > updatePartitionMetadata` and obtains the latest value corresponding to >> each >> > metric based on the tags using `ClientQuotaCallback.quotaLimit()`. >> > >> > An alternative may be to delay quota metrics updates until the next >> request >> > that uses the metric. When we get sensors, we can check if the quota >> > configured in the metric matches the value returned by ` >> > ClientQuotaCallback.quota()`. This will be slightly more expensive >> since we >> > need to check on every request, but the callback API as well as the >> quota >> > manager update code path would be simpler. What do you think? >> > >> > Thanks, >> > >> > Rajini >> > >> > >> > >> > On Wed, Mar 14, 2018 at 11:21 PM, Rajini Sivaram < >> rajinisiva...@gmail.com> >> > wrote: >> > >> > > Hi Jun, >> > > >> > > Thank you for reviewing the KIP. >> > > >> > > 10. This is the current behaviour (this KIP retains the same behaviour >> > for >> > > the default quota callback). We include 'user' and 'client-id' tags in >> > > all the quota metrics, rather than omit tags at the moment. >> > > >> > > 11. Ah, I hadn't realised that. I wasn't expecting to include deleted >> > > partitions in updatePartitionMetadata. I have updated the Javadoc in >> the >> > > KIP to reflect that. >> > > >> > > 12. When quotas are updated as a result of `updateQuota` or ` >> > > updatePartitionMetadata`, we may need to update quota bound for one or >> > > more existing metrics. I didn't want to expose metrics to the >> callback. >> > So ` >> > > quotaLimit` was providing the new quotas corresponding to existing >> > > metrics. But perhaps a neater way to do this is to return updated >> quotas >> > as >> > > the return value of `updateQuota` and `updatePartitionMetadata` so >> that >> > > the quota manager can handle metrics updates for those. I have updated >> > the >> > > KIP. >> > > >> > > >> > > On Wed, Mar 14, 2018 at 9:57 PM, Jun Rao <j...@confluent.io> wrote: >> > > >> > >> Hi, Rajini, >> > >> >> > >> Thanks for the KIP. Looks good overall. A few comments below. >> > >> >> > >> 10. "If <user> quota config is used, *user* tag is set to user >> principal >> > >> of >> > >> the session and *client-id* tag is set to empty string. " Could we >> just >> > >> omit such a tag if the value is empty? >> > >> >> > >> 11. I think Viktor has a valid point on handling partition removal. >> > >> Currently, we use -2 as the leader to signal the deletion of a >> > partition. >> > >> Not sure if we want to depend on that in the interface since it's an >> > >> internal value. >> > >> >> > >> 12. Could you explain a bit more the need for quotaLimit()? This is >> > called >> > >> after the updateQuota() call. Could we just let updateQuota do what >> > >> quotaLimit() >> > >> does? >> > >> >> > >> Jun >> > >> >> > >> On Wed, Feb 21, 2018 at 10:57 AM, Rajini Sivaram < >> > rajinisiva...@gmail.com >> > >> > >> > >> wrote: >> > >> >> > >> > Hi all, >> > >> > >> > >> > I have submitted KIP-257 to enable customisation of client quota >> > >> > computation: >> > >> > >> > >> > https://cwiki.apache.org/confluence/display/KAFKA/KIP- >> > >> > 257+-+Configurable+Quota+Management >> > >> > >> > >> > >> > >> > The KIP proposes to make quota management pluggable to enable >> > >> group-based >> > >> > and partition-based quotas for clients. >> > >> > >> > >> > Feedback and suggestions are welcome. >> > >> > >> > >> > Thank you... >> > >> > >> > >> > Regards, >> > >> > >> > >> > Rajini >> > >> > >> > >> >> > > >> > > >> > >> > >
