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 > > >> > > > >> > > > > > > > > >
