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