The quota callback interface was updated based on Jun's suggestion during the PR review. I have updated the KIP to reflect this.
Let me know if there are any concerns. Thanks, Rajini On Thu, Apr 5, 2018 at 1:04 PM, Rajini Sivaram <rajinisiva...@gmail.com> wrote: > Thanks, Ismael. > > I have updated the KIP and the PR. > > On Wed, Apr 4, 2018 at 7:37 PM, Ismael Juma <ism...@juma.me.uk> wrote: > >> Sounds good to me Rajini. Good catch spotting this before it's included in >> a release. :) >> >> Ismael >> >> On Wed, Apr 4, 2018 at 11:13 AM, Rajini Sivaram <rajinisiva...@gmail.com> >> wrote: >> >> > For compatibility reasons, we are now using Java rather than Scala for >> all >> > pluggable interfaces including those on the broker. There is already a >> KIP >> > to move Authorizer to Java as well. As we will be removing support for >> Java >> > 7 in the next release, we can also use default methods in Java when we >> need >> > to update pluggable Java interfaces. So the plan is to use Java for all >> new >> > pluggable interfaces. >> > >> > We already have the package org.apache.kafka.server, under which we have >> > the sub-package for policies, so it makes sense to define quota >> callback as >> > a Java interface here too. >> > >> > If there are any concerns, please let me know. Otherwise I will update >> the >> > KIP and the associated PR. >> > >> > Thank you, >> > >> > Rajini >> > >> > On Thu, Mar 22, 2018 at 9:52 PM, Rajini Sivaram < >> rajinisiva...@gmail.com> >> > wrote: >> > >> > > 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.quotaLimi >> t()`. >> > >>> > >> > >>> > 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 >> > >>> > >> > >> > >>> > >> >> > >>> > > >> > >>> > > >> > >>> > >> > >>> >> > >> >> > >> >> > > >> > >> > >
