Re: [DISCUSS] KIP-257 - Configurable Quota Management
Hi Ismael, Sorry, I had missed your note earlier. Jun had suggested changing the quota callback methods so that there is only one method to obtain quota limits. We used to have two methods, one which returned (*quotaLimit+metricTags)* and another which returned *quotaLimit* given the *metricTags.* Now the first method returns just the *metricTags* and we use the second to obtain *quotaLimit.* We have three scenarios: 1. Determine the quota for a request based on principal 2. Quota updated in ZK, we need to update quota limits on existing metrics 3. Quota updated externally (e.g. quotas are stored in a database or custom dynamic configs are used) For the default quota configuration using ZooKeeper, the two new methods work well. For 1) the first time we see a client, we get *metricTags* and use *quotaLimit* to get the limit for those tags and for subsequent requests, we just need *metricTags*. For 2) we get *quotaLimit* based on *metricTags* to update existing metrics. In many cases, this could just be a single metric update. For 3), however, I had to add another method to check if quotas need updating. The boolean method `quotaResetRequired()` is invoked when we are determining quotas for a connection, and it basically does whatever is done in 2) for externally managed quotas. On Fri, Apr 6, 2018 at 12:58 AM, Ismael Juma wrote: > 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 > 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 > > wrote: > > > > > Thanks, Ismael. > > > > > > I have updated the KIP and the PR. > > > > > > On Wed, Apr 4, 2018 at 7:37 PM, Ismael Juma 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 > 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 ` > > >
Re: [DISCUSS] KIP-257 - Configurable Quota Management
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 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 > wrote: > > > Thanks, Ismael. > > > > I have updated the KIP and the PR. > > > > On Wed, Apr 4, 2018 at 7:37 PM, Ismael Juma 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 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 > >
Re: [DISCUSS] KIP-257 - Configurable Quota Management
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 wrote: > Thanks, Ismael. > > I have updated the KIP and the PR. > > On Wed, Apr 4, 2018 at 7:37 PM, Ismael Juma 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 >> 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 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 >> >
Re: [DISCUSS] KIP-257 - Configurable Quota Management
Thanks, Ismael. I have updated the KIP and the PR. On Wed, Apr 4, 2018 at 7:37 PM, Ismael Juma 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 > 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 > > > 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 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 upd
Re: [DISCUSS] KIP-257 - Configurable Quota Management
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 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 > 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 > > > 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 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 `updat
Re: [DISCUSS] KIP-257 - Configurable Quota Management
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 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 > 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 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 >> > >>> 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 wrote: >>> > > >>> > >> Hi, Rajini, >>> > >> >>> > >> Thanks for the KIP. Looks good overall. A few comments below. >>> > >> >>> > >> 10. "If quota config is used, *user* tag is set to user >>> principal >>> > >> of >>> > >> the se
Re: [DISCUSS] KIP-257 - Configurable Quota Management
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 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 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 >> 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 wrote: >> > > >> > >> Hi, Rajini, >> > >> >> > >> Thanks for the KIP. Looks good overall. A few comments below. >> > >> >> > >> 10. "If 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
Re: [DISCUSS] KIP-257 - Configurable Quota Management
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 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 > 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 wrote: > > > > > >> Hi, Rajini, > > >> > > >> Thanks for the KIP. Looks good overall. A few comments below. > > >> > > >> 10. "If 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. > > >> > > > >> > T
Re: [DISCUSS] KIP-257 - Configurable Quota Management
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 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 > 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 wrote: > > > >> Hi, Rajini, > >> > >> Thanks for the KIP. Looks good overall. A few comments below. > >> > >> 10. "If 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 > >> > > >> > > > > >
Re: [DISCUSS] KIP-257 - Configurable Quota Management
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 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 wrote: > >> Hi, Rajini, >> >> Thanks for the KIP. Looks good overall. A few comments below. >> >> 10. "If 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 > > >> 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 >> > >> > >
Re: [DISCUSS] KIP-257 - Configurable Quota Management
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 wrote: > Hi, Rajini, > > Thanks for the KIP. Looks good overall. A few comments below. > > 10. "If 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 > 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 > > >
Re: [DISCUSS] KIP-257 - Configurable Quota Management
Hi, Rajini, Thanks for the KIP. Looks good overall. A few comments below. 10. "If 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 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 >
Re: [DISCUSS] KIP-257 - Configurable Quota Management
Hi Rajini, Well, I didn't really have specific use cases for having other metadata (like isr and replicas), just thought it would be a more robust interface. But yea, since currently there are no specific use cases, it makes sense not to include them. Viktor On Tue, Mar 13, 2018 at 10:47 AM, Rajini Sivaram wrote: > I have submitted a PR with the changes proposed in this KIP ( > https://github.com/apache/kafka/pull/4699). I added an additional method > to > the quota callback in the KIP to simplify metrics updates when quotas are > updated. > > Feedback and suggestions are welcome. If there are no other concerns, I > will start vote later this week. > > Thank you, > > Rajini > > On Wed, Mar 7, 2018 at 12:34 PM, Rajini Sivaram > wrote: > > > Hi Viktor, > > > > Thanks for reviewing the KIP. > > > > 1. Yes, that is correct. Typically quotas would depend only on the > current > > partition state. But if you did want to track deleted partitions, you can > > calculate the diff. > > 2. I can't think of an use case where ISRs or other replica information > > would be useful to configure quotas. Since partition leaders process > > fetch/produce requests, this is clearly useful in terms of setting > quotas. > > But I have defined PartitionMetadata trait rather than just using the > > leader as an int so that we can add additional methods in future if > > required. This keeps the interface extensible. Did you have any use case > in > > mind where additional metadata would be useful? > > > > Regards, > > > > Rajini > > > > On Tue, Mar 6, 2018 at 8:56 AM, Viktor Somogyi > > wrote: > > > >> Hi Rajini, > >> > >> I've read through your KIP and it looks good, I only have two things to > >> clarify. > >> 1. How do we detect removed partitions in updatePartitionMetadata? I'm > >> presuming that the list here is the currently existing map of > partitions, > >> so if something is removed it can be calculated as the diff of the > current > >> and the previous update. Is that right? > >> 2. PartitionMetadata contains only the leader at this moment, however > >> there > >> are similar classes that contain more information, like the replicas, > isr, > >> offline replicas. I think including them might make sense to provide a > >> more > >> robust API. What do you think? > >> > >> Thanks, > >> Viktor > >> > >> On Wed, Feb 21, 2018 at 7:57 PM, 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 > >> > > >> > > > > >
Re: [DISCUSS] KIP-257 - Configurable Quota Management
I have submitted a PR with the changes proposed in this KIP ( https://github.com/apache/kafka/pull/4699). I added an additional method to the quota callback in the KIP to simplify metrics updates when quotas are updated. Feedback and suggestions are welcome. If there are no other concerns, I will start vote later this week. Thank you, Rajini On Wed, Mar 7, 2018 at 12:34 PM, Rajini Sivaram wrote: > Hi Viktor, > > Thanks for reviewing the KIP. > > 1. Yes, that is correct. Typically quotas would depend only on the current > partition state. But if you did want to track deleted partitions, you can > calculate the diff. > 2. I can't think of an use case where ISRs or other replica information > would be useful to configure quotas. Since partition leaders process > fetch/produce requests, this is clearly useful in terms of setting quotas. > But I have defined PartitionMetadata trait rather than just using the > leader as an int so that we can add additional methods in future if > required. This keeps the interface extensible. Did you have any use case in > mind where additional metadata would be useful? > > Regards, > > Rajini > > On Tue, Mar 6, 2018 at 8:56 AM, Viktor Somogyi > wrote: > >> Hi Rajini, >> >> I've read through your KIP and it looks good, I only have two things to >> clarify. >> 1. How do we detect removed partitions in updatePartitionMetadata? I'm >> presuming that the list here is the currently existing map of partitions, >> so if something is removed it can be calculated as the diff of the current >> and the previous update. Is that right? >> 2. PartitionMetadata contains only the leader at this moment, however >> there >> are similar classes that contain more information, like the replicas, isr, >> offline replicas. I think including them might make sense to provide a >> more >> robust API. What do you think? >> >> Thanks, >> Viktor >> >> On Wed, Feb 21, 2018 at 7:57 PM, Rajini Sivaram >> 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 >> > >> > >
Re: [DISCUSS] KIP-257 - Configurable Quota Management
Hi Viktor, Thanks for reviewing the KIP. 1. Yes, that is correct. Typically quotas would depend only on the current partition state. But if you did want to track deleted partitions, you can calculate the diff. 2. I can't think of an use case where ISRs or other replica information would be useful to configure quotas. Since partition leaders process fetch/produce requests, this is clearly useful in terms of setting quotas. But I have defined PartitionMetadata trait rather than just using the leader as an int so that we can add additional methods in future if required. This keeps the interface extensible. Did you have any use case in mind where additional metadata would be useful? Regards, Rajini On Tue, Mar 6, 2018 at 8:56 AM, Viktor Somogyi wrote: > Hi Rajini, > > I've read through your KIP and it looks good, I only have two things to > clarify. > 1. How do we detect removed partitions in updatePartitionMetadata? I'm > presuming that the list here is the currently existing map of partitions, > so if something is removed it can be calculated as the diff of the current > and the previous update. Is that right? > 2. PartitionMetadata contains only the leader at this moment, however there > are similar classes that contain more information, like the replicas, isr, > offline replicas. I think including them might make sense to provide a more > robust API. What do you think? > > Thanks, > Viktor > > On Wed, Feb 21, 2018 at 7:57 PM, Rajini Sivaram > 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 > > >
Re: [DISCUSS] KIP-257 - Configurable Quota Management
Hi Rajini, I've read through your KIP and it looks good, I only have two things to clarify. 1. How do we detect removed partitions in updatePartitionMetadata? I'm presuming that the list here is the currently existing map of partitions, so if something is removed it can be calculated as the diff of the current and the previous update. Is that right? 2. PartitionMetadata contains only the leader at this moment, however there are similar classes that contain more information, like the replicas, isr, offline replicas. I think including them might make sense to provide a more robust API. What do you think? Thanks, Viktor On Wed, Feb 21, 2018 at 7:57 PM, Rajini Sivaram 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 >
