Hi Rajini, Can you share the motivation for the change? Not sure if the new approach is better.
Ismael On Thu, Apr 5, 2018 at 4:06 PM, Rajini Sivaram <rajinisiva...@gmail.com> wrote: > 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 > >> > >>> > >> > > >> > >>> > >> > >> > >>> > > > >> > >>> > > > >> > >>> > > >> > >>> > >> > >> > >> > >> > >> > > > >> > > >> > > > > >
