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