Hi, David, Thanks for the explanation. The KIP looks good to me now.
Jun On Tue, Jun 9, 2020 at 4:27 AM David Jacot <dja...@confluent.io> wrote: > Hi Jun, > > 40. Yes, ThrottleTimeMs is set when the error code is set to QuotaViolated. > This > is required to let the client know how long it must wait. This is explained > in the > "Handling of new/old clients". > > Best, > David > > On Mon, Jun 8, 2020 at 9:29 PM Jun Rao <j...@confluent.io> wrote: > > > Hi, David, > > > > Thanks for the updated KIP. Another minor comment below. > > > > 40. For the new `QUOTA_VIOLATED` error in the response to > > CreateTopics/CreatePartitions/DeleteTopics, could you clarify > > whether ThrottleTimeMs is set when the error code is set to > QUOTA_VIOLATED? > > > > Jun > > > > On Mon, Jun 8, 2020 at 9:32 AM David Jacot <dja...@confluent.io> wrote: > > > > > Hi Jun, > > > > > > 30. The rate is accumulated at the partition level. Let me clarify this > > in > > > the KIP. > > > > > > Best, > > > David > > > > > > On Sat, Jun 6, 2020 at 2:37 AM Anna Povzner <a...@confluent.io> wrote: > > > > > > > Hi David, > > > > > > > > The KIP looks good to me. I am going to the voting thread... > > > > > > > > Hi Jun, > > > > > > > > Yes, exactly. That's a separate thing from this KIP, so working on > the > > > fix. > > > > > > > > Thanks, > > > > Anna > > > > > > > > On Fri, Jun 5, 2020 at 4:36 PM Jun Rao <j...@confluent.io> wrote: > > > > > > > > > Hi, Anna, > > > > > > > > > > Thanks for the comment. For the problem that you described, perhaps > > we > > > > need > > > > > to make the quota checking and recording more atomic? > > > > > > > > > > Hi, David, > > > > > > > > > > Thanks for the updated KIP. Looks good to me now. Just one minor > > > comment > > > > > below. > > > > > > > > > > 30. controller_mutations_rate: For topic creation and deletion, is > > the > > > > rate > > > > > accumulated at the topic or partition level? It would be useful to > > make > > > > it > > > > > clear in the wiki. > > > > > > > > > > Jun > > > > > > > > > > On Fri, Jun 5, 2020 at 7:23 AM David Jacot <dja...@confluent.io> > > > wrote: > > > > > > > > > > > Hi Anna and Jun, > > > > > > > > > > > > You are right. We should allocate up to the quota for each old > > > sample. > > > > > > > > > > > > I have revamped the Throttling Algorithm section to better > explain > > > our > > > > > > thought process and the token bucket inspiration. > > > > > > > > > > > > I have also added a chapter with few guidelines about how to > define > > > > > > the quota. There is no magic formula for this but I give few > > > insights. > > > > > > I don't have specific numbers that can be used out of the box so > I > > > > > > think that it is better to not put any for the time being. We can > > > > always > > > > > > complement later on in the documentation. > > > > > > > > > > > > Please, take a look and let me know what you think. > > > > > > > > > > > > Cheers, > > > > > > David > > > > > > > > > > > > On Fri, Jun 5, 2020 at 8:37 AM Anna Povzner <a...@confluent.io> > > > wrote: > > > > > > > > > > > > > Hi David and Jun, > > > > > > > > > > > > > > I dug a bit deeper into the Rate implementation, and wanted to > > > > confirm > > > > > > that > > > > > > > I do believe that the token bucket behavior is better for the > > > reasons > > > > > we > > > > > > > already discussed but wanted to summarize. The main difference > > > > between > > > > > > Rate > > > > > > > and token bucket is that the Rate implementation allows a burst > > by > > > > > > > borrowing from the future, whereas a token bucket allows a > burst > > by > > > > > using > > > > > > > accumulated tokens from the previous idle period. Using > > accumulated > > > > > > tokens > > > > > > > smoothes out the rate measurement in general. Configuring a > large > > > > burst > > > > > > > requires configuring a large quota window, which causes long > > delays > > > > for > > > > > > > bursty workload, due to borrowing credits from the future. > > Perhaps > > > it > > > > > is > > > > > > > useful to add a summary in the beginning of the Throttling > > > Algorithm > > > > > > > section? > > > > > > > > > > > > > > In my previous email, I mentioned the issue we observed with > the > > > > > > bandwidth > > > > > > > quota, where a low quota (1MB/s per broker) was limiting > > bandwidth > > > > > > visibly > > > > > > > below the quota. I thought it was strictly the issue with the > > Rate > > > > > > > implementation as well, but I found a root cause to be > different > > > but > > > > > > > amplified by the Rate implementation (long throttle delays of > > > > requests > > > > > > in a > > > > > > > burst). I will describe it here for completeness using the > > > following > > > > > > > example: > > > > > > > > > > > > > > - > > > > > > > > > > > > > > Quota = 1MB/s, default window size and number of samples > > > > > > > - > > > > > > > > > > > > > > Suppose there are 6 connections (maximum 6 outstanding > > > requests), > > > > > and > > > > > > > each produce request is 5MB. If all requests arrive in a > > burst, > > > > the > > > > > > > last 4 > > > > > > > requests (20MB over 10MB allowed in a window) may get the > same > > > > > > throttle > > > > > > > time if they are processed concurrently. We record the rate > > > under > > > > > the > > > > > > > lock, > > > > > > > but then calculate throttle time separately after that. So, > > for > > > > each > > > > > > > request, the observed rate could be 3MB/s, and each request > > gets > > > > > > > throttle > > > > > > > delay = 20 seconds (instead of 5, 10, 15, 20 respectively). > > The > > > > > delay > > > > > > is > > > > > > > longer than the total rate window, which results in lower > > > > bandwidth > > > > > > than > > > > > > > the quota. Since all requests got the same delay, they will > > also > > > > > > arrive > > > > > > > in > > > > > > > a burst, which may also result in longer delay than > necessary. > > > It > > > > > > looks > > > > > > > pretty easy to fix, so I will open a separate JIRA for it. > > This > > > > can > > > > > be > > > > > > > additionally mitigated by token bucket behavior. > > > > > > > > > > > > > > > > > > > > > For the algorithm "So instead of having one sample equal to 560 > > in > > > > the > > > > > > last > > > > > > > window, we will have 100 samples equal to 5.6.", I agree with > > Jun. > > > I > > > > > > would > > > > > > > allocate 5 per each old sample that is still in the overall > > window. > > > > It > > > > > > > would be a bit larger granularity than the pure token bucket > (we > > > > lose 5 > > > > > > > units / mutation once we move past the sample window), but it > is > > > > better > > > > > > > than the long delay. > > > > > > > > > > > > > > Thanks, > > > > > > > > > > > > > > Anna > > > > > > > > > > > > > > > > > > > > > On Thu, Jun 4, 2020 at 6:33 PM Jun Rao <j...@confluent.io> > wrote: > > > > > > > > > > > > > > > Hi, David, Anna, > > > > > > > > > > > > > > > > Thanks for the discussion and the updated wiki. > > > > > > > > > > > > > > > > 11. If we believe the token bucket behavior is better in > terms > > of > > > > > > > handling > > > > > > > > the burst behavior, we probably don't need a separate KIP > since > > > > it's > > > > > > just > > > > > > > > an implementation detail. > > > > > > > > > > > > > > > > Regarding "So instead of having one sample equal to 560 in > the > > > last > > > > > > > window, > > > > > > > > we will have 100 samples equal to 5.6.", I was thinking that > we > > > > will > > > > > > > > allocate 5 to each of the first 99 samples and 65 to the last > > > > sample. > > > > > > > Then, > > > > > > > > 6 new samples have to come before the balance becomes 0 > again. > > > > > > > Intuitively, > > > > > > > > we are accumulating credits in each sample. If a usage comes > > in, > > > we > > > > > > first > > > > > > > > use all existing credits to offset that. If we can't, the > > > remaining > > > > > > usage > > > > > > > > will be recorded in the last sample, which will be offset by > > > future > > > > > > > > credits. That seems to match the token bucket behavior the > > > closest. > > > > > > > > > > > > > > > > 20. Could you provide some guidelines on the typical rate > that > > an > > > > > admin > > > > > > > > should set? > > > > > > > > > > > > > > > > Jun > > > > > > > > > > > > > > > > On Thu, Jun 4, 2020 at 8:22 AM David Jacot < > > dja...@confluent.io> > > > > > > wrote: > > > > > > > > > > > > > > > > > Hi all, > > > > > > > > > > > > > > > > > > I just published an updated version of the KIP which > > includes: > > > > > > > > > * Using a slightly modified version of our Rate. I have > tried > > > to > > > > > > > > formalize > > > > > > > > > it based on our discussion. As Anna suggested, we may find > a > > > > better > > > > > > way > > > > > > > > to > > > > > > > > > implement it. > > > > > > > > > * Handling of ValidateOnly as pointed out by Tom. > > > > > > > > > > > > > > > > > > Please, check it out and let me know what you think. > > > > > > > > > > > > > > > > > > Best, > > > > > > > > > David > > > > > > > > > > > > > > > > > > On Thu, Jun 4, 2020 at 4:57 PM Tom Bentley < > > > tbent...@redhat.com> > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > Hi David, > > > > > > > > > > > > > > > > > > > > As a user I might expect the validateOnly option to do > > > > everything > > > > > > > > except > > > > > > > > > > actually make the changes. That interpretation would > imply > > > the > > > > > > quota > > > > > > > > > should > > > > > > > > > > be checked, but the check should obviously be side-effect > > > > free. I > > > > > > > think > > > > > > > > > > this interpretation could be useful because it gives the > > > caller > > > > > > > either > > > > > > > > > some > > > > > > > > > > confidence that they're not going to hit the quota, or > tell > > > > them, > > > > > > via > > > > > > > > the > > > > > > > > > > exception, when they can expect the call to work. But for > > > this > > > > to > > > > > > be > > > > > > > > > useful > > > > > > > > > > it would require the retry logic to not retry the request > > > when > > > > > > > > > validateOnly > > > > > > > > > > was set. > > > > > > > > > > > > > > > > > > > > On the other hand, if validateOnly is really about > > validating > > > > > only > > > > > > > some > > > > > > > > > > aspects of the request (which maybe is what the name > > > implies), > > > > > then > > > > > > > we > > > > > > > > > > should clarify in the Javadoc that the quota is not > > included > > > in > > > > > the > > > > > > > > > > validation. > > > > > > > > > > > > > > > > > > > > On balance, I agree with what you're proposing, since the > > > extra > > > > > > > utility > > > > > > > > > of > > > > > > > > > > including the quota in the validation seems to be not > worth > > > the > > > > > > extra > > > > > > > > > > complication for the retry. > > > > > > > > > > > > > > > > > > > > Thanks, > > > > > > > > > > > > > > > > > > > > Tom > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Thu, Jun 4, 2020 at 3:32 PM David Jacot < > > > > dja...@confluent.io> > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > Hi Tom, > > > > > > > > > > > > > > > > > > > > > > That's a good question. As the validation does not > create > > > any > > > > > > load > > > > > > > on > > > > > > > > > the > > > > > > > > > > > controller, I was planning to do it without checking > the > > > > quota > > > > > at > > > > > > > > all. > > > > > > > > > > Does > > > > > > > > > > > that > > > > > > > > > > > sound reasonable? > > > > > > > > > > > > > > > > > > > > > > Best, > > > > > > > > > > > David > > > > > > > > > > > > > > > > > > > > > > On Thu, Jun 4, 2020 at 4:23 PM David Jacot < > > > > > dja...@confluent.io> > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > Hi Jun and Anna, > > > > > > > > > > > > > > > > > > > > > > > > Thank you both for your replies. > > > > > > > > > > > > > > > > > > > > > > > > Based on our recent discussion, I agree that using a > > rate > > > > is > > > > > > > better > > > > > > > > > to > > > > > > > > > > > > remain > > > > > > > > > > > > consistent with other quotas. As you both suggested, > it > > > > seems > > > > > > > that > > > > > > > > > > > changing > > > > > > > > > > > > the way we compute the rate to better handle spiky > > > > workloads > > > > > > and > > > > > > > > > > behave a > > > > > > > > > > > > bit more similarly to the token bucket algorithm > makes > > > > sense > > > > > > for > > > > > > > > all > > > > > > > > > > > > quotas as > > > > > > > > > > > > well. > > > > > > > > > > > > > > > > > > > > > > > > I will update the KIP to reflect this. > > > > > > > > > > > > > > > > > > > > > > > > Anna, I think that we can explain this in this KIP. > We > > > > can't > > > > > > just > > > > > > > > say > > > > > > > > > > > that > > > > > > > > > > > > the Rate > > > > > > > > > > > > will be updated in this KIP. I think that we need to > > > give a > > > > > bit > > > > > > > > more > > > > > > > > > > > info. > > > > > > > > > > > > > > > > > > > > > > > > Best, > > > > > > > > > > > > David > > > > > > > > > > > > > > > > > > > > > > > > On Thu, Jun 4, 2020 at 6:31 AM Anna Povzner < > > > > > a...@confluent.io > > > > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > >> Hi Jun and David, > > > > > > > > > > > >> > > > > > > > > > > > >> Regarding token bucket vs, Rate behavior. We > recently > > > > > > observed a > > > > > > > > > > couple > > > > > > > > > > > of > > > > > > > > > > > >> cases where a bursty workload behavior would result > in > > > > > > long-ish > > > > > > > > > pauses > > > > > > > > > > > in > > > > > > > > > > > >> between, resulting in lower overall bandwidth than > the > > > > > quota. > > > > > > I > > > > > > > > will > > > > > > > > > > > need > > > > > > > > > > > >> to debug this a bit more to be 100% sure, but it > does > > > look > > > > > > like > > > > > > > > the > > > > > > > > > > case > > > > > > > > > > > >> described by David earlier in this thread. So, I > agree > > > > with > > > > > > Jun > > > > > > > > -- I > > > > > > > > > > > think > > > > > > > > > > > >> we should make all quota rate behavior consistent, > and > > > > > > probably > > > > > > > > > > similar > > > > > > > > > > > to > > > > > > > > > > > >> the token bucket one. > > > > > > > > > > > >> > > > > > > > > > > > >> Looking at KIP-13, it doesn't describe rate > > calculation > > > in > > > > > > > enough > > > > > > > > > > > detail, > > > > > > > > > > > >> but does mention window size. So, we could keep > > "window > > > > > size" > > > > > > > and > > > > > > > > > > > "number > > > > > > > > > > > >> of samples" configs and change Rate implementation > to > > be > > > > > more > > > > > > > > > similar > > > > > > > > > > to > > > > > > > > > > > >> token bucket: > > > > > > > > > > > >> * number of samples define our burst size > > > > > > > > > > > >> * Change the behavior, which could be described as: > > If a > > > > > burst > > > > > > > > > happens > > > > > > > > > > > >> after an idle period, the burst would effectively > > spread > > > > > > evenly > > > > > > > > over > > > > > > > > > > the > > > > > > > > > > > >> (now - window) time period, where window is (<number > > of > > > > > > > samples> - > > > > > > > > > 1)* > > > > > > > > > > > >> <window size>. Which basically describes a token > > bucket, > > > > > while > > > > > > > > > keeping > > > > > > > > > > > the > > > > > > > > > > > >> current quota configs. I think we can even implement > > > this > > > > by > > > > > > > > > changing > > > > > > > > > > > the > > > > > > > > > > > >> way we record the last sample or lastWindowMs. > > > > > > > > > > > >> > > > > > > > > > > > >> Jun, if we would be changing Rate calculation > behavior > > > in > > > > > > > > bandwidth > > > > > > > > > > and > > > > > > > > > > > >> request quotas, would we need a separate KIP? > > Shouldn't > > > > need > > > > > > to > > > > > > > if > > > > > > > > > we > > > > > > > > > > > >> keep window size and number of samples configs, > right? > > > > > > > > > > > >> > > > > > > > > > > > >> Thanks, > > > > > > > > > > > >> Anna > > > > > > > > > > > >> > > > > > > > > > > > >> On Wed, Jun 3, 2020 at 3:24 PM Jun Rao < > > > j...@confluent.io> > > > > > > > wrote: > > > > > > > > > > > >> > > > > > > > > > > > >> > Hi, David, > > > > > > > > > > > >> > > > > > > > > > > > > >> > Thanks for the reply. > > > > > > > > > > > >> > > > > > > > > > > > > >> > 11. To match the behavior in the Token bucket > > > approach, > > > > I > > > > > > was > > > > > > > > > > thinking > > > > > > > > > > > >> that > > > > > > > > > > > >> > requests that don't fit in the previous time > windows > > > > will > > > > > be > > > > > > > > > > > >> accumulated in > > > > > > > > > > > >> > the current time window. So, the 60 extra requests > > > will > > > > be > > > > > > > > > > accumulated > > > > > > > > > > > >> in > > > > > > > > > > > >> > the latest window. Then, the client also has to > wait > > > for > > > > > 12 > > > > > > > more > > > > > > > > > > secs > > > > > > > > > > > >> > before throttling is removed. I agree that this is > > > > > probably > > > > > > a > > > > > > > > > better > > > > > > > > > > > >> > behavior and it's reasonable to change the > existing > > > > > behavior > > > > > > > to > > > > > > > > > this > > > > > > > > > > > >> one. > > > > > > > > > > > >> > > > > > > > > > > > > >> > To me, it seems that sample_size * num_windows is > > the > > > > same > > > > > > as > > > > > > > > max > > > > > > > > > > > burst > > > > > > > > > > > >> > balance. The latter seems a bit better to > configure. > > > The > > > > > > thing > > > > > > > > is > > > > > > > > > > that > > > > > > > > > > > >> the > > > > > > > > > > > >> > existing quota system has already been used in > > quite a > > > > few > > > > > > > > places > > > > > > > > > > and > > > > > > > > > > > >> if we > > > > > > > > > > > >> > want to change the configuration in the future, > > there > > > is > > > > > the > > > > > > > > > > migration > > > > > > > > > > > >> > cost. Given that, do you feel it's better to adopt > > the > > > > > new > > > > > > > > token > > > > > > > > > > > bucket > > > > > > > > > > > >> > terminology or just adopt the behavior somehow > into > > > our > > > > > > > existing > > > > > > > > > > > >> system? If > > > > > > > > > > > >> > it's the former, it would be useful to document > this > > > in > > > > > the > > > > > > > > > rejected > > > > > > > > > > > >> > section and add a future plan on migrating > existing > > > > quota > > > > > > > > > > > >> configurations. > > > > > > > > > > > >> > > > > > > > > > > > > >> > Jun > > > > > > > > > > > >> > > > > > > > > > > > > >> > > > > > > > > > > > > >> > On Tue, Jun 2, 2020 at 6:55 AM David Jacot < > > > > > > > dja...@confluent.io > > > > > > > > > > > > > > > > > > > > wrote: > > > > > > > > > > > >> > > > > > > > > > > > > >> > > Hi Jun, > > > > > > > > > > > >> > > > > > > > > > > > > > >> > > Thanks for your reply. > > > > > > > > > > > >> > > > > > > > > > > > > > >> > > 10. I think that both options are likely > > equivalent > > > > from > > > > > > an > > > > > > > > > > accuracy > > > > > > > > > > > >> > point > > > > > > > > > > > >> > > of > > > > > > > > > > > >> > > view. If we put the implementation aside, > > > > conceptually, > > > > > I > > > > > > am > > > > > > > > not > > > > > > > > > > > >> > convinced > > > > > > > > > > > >> > > by the used based approach because resources > don't > > > > have > > > > > a > > > > > > > > clear > > > > > > > > > > > owner > > > > > > > > > > > >> > > in AK at the moment. A topic can be created by > > > > > (Principal > > > > > > A, > > > > > > > > no > > > > > > > > > > > client > > > > > > > > > > > >> > id), > > > > > > > > > > > >> > > then can be extended by (no principal, Client > B), > > > and > > > > > > > finally > > > > > > > > > > > deleted > > > > > > > > > > > >> by > > > > > > > > > > > >> > > (Principal C, Client C). This does not sound > right > > > to > > > > me > > > > > > > and I > > > > > > > > > > fear > > > > > > > > > > > >> that > > > > > > > > > > > >> > it > > > > > > > > > > > >> > > is not going to be easy to grasp for our users. > > > > > > > > > > > >> > > > > > > > > > > > > > >> > > Regarding the naming, I do agree that we can > make > > it > > > > > more > > > > > > > > future > > > > > > > > > > > >> proof. > > > > > > > > > > > >> > > I propose `controller_mutations_rate`. I think > > that > > > > > > > > > > differentiating > > > > > > > > > > > >> the > > > > > > > > > > > >> > > mutations > > > > > > > > > > > >> > > from the reads is still a good thing for the > > future. > > > > > > > > > > > >> > > > > > > > > > > > > > >> > > 11. I am not convinced by your proposal for the > > > > > following > > > > > > > > > reasons: > > > > > > > > > > > >> > > > > > > > > > > > > > >> > > First, in my toy example, I used 101 windows and > > 7 * > > > > 80 > > > > > > > > > requests. > > > > > > > > > > We > > > > > > > > > > > >> > could > > > > > > > > > > > >> > > effectively allocate 5 * 100 requests to the > > > previous > > > > > > > windows > > > > > > > > > > > assuming > > > > > > > > > > > >> > that > > > > > > > > > > > >> > > they are empty. What shall we do with the > > remaining > > > 60 > > > > > > > > requests? > > > > > > > > > > > >> Shall we > > > > > > > > > > > >> > > allocate them to the current window or shall we > > > divide > > > > > it > > > > > > > > among > > > > > > > > > > all > > > > > > > > > > > >> the > > > > > > > > > > > >> > > windows? > > > > > > > > > > > >> > > > > > > > > > > > > > >> > > Second, I don't think that we can safely change > > the > > > > > > behavior > > > > > > > > of > > > > > > > > > > all > > > > > > > > > > > >> the > > > > > > > > > > > >> > > existing > > > > > > > > > > > >> > > rates used because it actually changes the > > > computation > > > > > of > > > > > > > the > > > > > > > > > rate > > > > > > > > > > > as > > > > > > > > > > > >> > > values > > > > > > > > > > > >> > > allocated to past windows would expire before > they > > > > would > > > > > > > > today. > > > > > > > > > > > >> > > > > > > > > > > > > > >> > > Overall, while trying to fit in the current > rate, > > we > > > > are > > > > > > > going > > > > > > > > > to > > > > > > > > > > > >> build a > > > > > > > > > > > >> > > slightly > > > > > > > > > > > >> > > different version of the rate which will be even > > > more > > > > > > > > confusing > > > > > > > > > > for > > > > > > > > > > > >> > users. > > > > > > > > > > > >> > > > > > > > > > > > > > >> > > Instead, I think that we should embrace the > notion > > > of > > > > > > burst > > > > > > > as > > > > > > > > > it > > > > > > > > > > > >> could > > > > > > > > > > > >> > > also > > > > > > > > > > > >> > > be applied to other quotas in the future. Users > > > don't > > > > > have > > > > > > > to > > > > > > > > > know > > > > > > > > > > > >> that > > > > > > > > > > > >> > we > > > > > > > > > > > >> > > use the Token Bucket or a special rate inside at > > the > > > > end > > > > > > of > > > > > > > > the > > > > > > > > > > day. > > > > > > > > > > > >> It > > > > > > > > > > > >> > is > > > > > > > > > > > >> > > an > > > > > > > > > > > >> > > implementation detail. > > > > > > > > > > > >> > > > > > > > > > > > > > >> > > Users would be able to define: > > > > > > > > > > > >> > > - a rate R; and > > > > > > > > > > > >> > > - a maximum burst B. > > > > > > > > > > > >> > > > > > > > > > > > > > >> > > If we change the metrics to be as follow: > > > > > > > > > > > >> > > - the actual rate; > > > > > > > > > > > >> > > - the burst balance in %, 0 meaning that the > user > > is > > > > > > > > throttled; > > > > > > > > > > > >> > > It remains disattach from the algorithm. > > > > > > > > > > > >> > > > > > > > > > > > > > >> > > I personally prefer this over having to define a > > > rate > > > > > and > > > > > > a > > > > > > > > > number > > > > > > > > > > > of > > > > > > > > > > > >> > > windows > > > > > > > > > > > >> > > while having to understand that the number of > > > windows > > > > > > > > implicitly > > > > > > > > > > > >> defines > > > > > > > > > > > >> > > the > > > > > > > > > > > >> > > allowed burst. I think that it is clearer and > > easier > > > > to > > > > > > > grasp. > > > > > > > > > > Don't > > > > > > > > > > > >> you? > > > > > > > > > > > >> > > > > > > > > > > > > > >> > > Best, > > > > > > > > > > > >> > > David > > > > > > > > > > > >> > > > > > > > > > > > > > >> > > On Fri, May 29, 2020 at 6:38 PM Jun Rao < > > > > > j...@confluent.io > > > > > > > > > > > > > > > > wrote: > > > > > > > > > > > >> > > > > > > > > > > > > > >> > > > Hi, David, Anna, > > > > > > > > > > > >> > > > > > > > > > > > > > > >> > > > Thanks for the response. Sorry for the late > > reply. > > > > > > > > > > > >> > > > > > > > > > > > > > > >> > > > 10. Regarding exposing rate or usage as quota. > > > Your > > > > > > > argument > > > > > > > > > is > > > > > > > > > > > that > > > > > > > > > > > >> > > usage > > > > > > > > > > > >> > > > is not very accurate anyway and is harder to > > > > > implement. > > > > > > > So, > > > > > > > > > > let's > > > > > > > > > > > >> just > > > > > > > > > > > >> > > be a > > > > > > > > > > > >> > > > bit loose and expose rate. I am sort of > neutral > > on > > > > > that. > > > > > > > (1) > > > > > > > > > It > > > > > > > > > > > >> seems > > > > > > > > > > > >> > to > > > > > > > > > > > >> > > me > > > > > > > > > > > >> > > > that overall usage will be relatively more > > > accurate > > > > > than > > > > > > > > rate. > > > > > > > > > > All > > > > > > > > > > > >> the > > > > > > > > > > > >> > > > issues that Anna brought up seem to exist for > > rate > > > > > too. > > > > > > > Rate > > > > > > > > > has > > > > > > > > > > > the > > > > > > > > > > > >> > > > additional problem that the cost of each > request > > > may > > > > > not > > > > > > > be > > > > > > > > > > > uniform. > > > > > > > > > > > >> > (2) > > > > > > > > > > > >> > > In > > > > > > > > > > > >> > > > terms of implementation, a usage based > approach > > > > > requires > > > > > > > > > > tracking > > > > > > > > > > > >> the > > > > > > > > > > > >> > > user > > > > > > > > > > > >> > > > info through the life cycle of an operation. > > > > However, > > > > > as > > > > > > > you > > > > > > > > > > > >> mentioned, > > > > > > > > > > > >> > > > things like topic creation can generate > > additional > > > > > > > > > > > >> > > > LeaderAndIsr/UpdateMetadata requests. Longer > > term, > > > > we > > > > > > > > probably > > > > > > > > > > > want > > > > > > > > > > > >> to > > > > > > > > > > > >> > > > associate those cost to the user who initiated > > the > > > > > > > > operation. > > > > > > > > > If > > > > > > > > > > > we > > > > > > > > > > > >> do > > > > > > > > > > > >> > > > that, we sort of need to track the user for > the > > > full > > > > > > life > > > > > > > > > cycle > > > > > > > > > > of > > > > > > > > > > > >> the > > > > > > > > > > > >> > > > processing of an operation anyway. (3) If you > > > prefer > > > > > > rate > > > > > > > > > > > strongly, > > > > > > > > > > > >> I > > > > > > > > > > > >> > > don't > > > > > > > > > > > >> > > > have strong objections. However, I do feel > that > > > the > > > > > new > > > > > > > > quota > > > > > > > > > > name > > > > > > > > > > > >> > should > > > > > > > > > > > >> > > > be able to cover all controller related cost > > > longer > > > > > > term. > > > > > > > > This > > > > > > > > > > KIP > > > > > > > > > > > >> > > > currently only covers topic creation/deletion. > > It > > > > > would > > > > > > > not > > > > > > > > be > > > > > > > > > > > >> ideal if > > > > > > > > > > > >> > > in > > > > > > > > > > > >> > > > the future, we have to add yet another type of > > > quota > > > > > for > > > > > > > > some > > > > > > > > > > > other > > > > > > > > > > > >> > > > controller related operations. The quota name > in > > > the > > > > > KIP > > > > > > > has > > > > > > > > > > > >> partition > > > > > > > > > > > >> > > > mutation. In the future, if we allow things > like > > > > topic > > > > > > > > > renaming, > > > > > > > > > > > it > > > > > > > > > > > >> may > > > > > > > > > > > >> > > not > > > > > > > > > > > >> > > > be related to partition mutation directly and > it > > > > would > > > > > > be > > > > > > > > > > trickier > > > > > > > > > > > >> to > > > > > > > > > > > >> > fit > > > > > > > > > > > >> > > > those operations in the current quota. So, > maybe > > > sth > > > > > > more > > > > > > > > > > general > > > > > > > > > > > >> like > > > > > > > > > > > >> > > > controller_operations_quota will be more > future > > > > proof. > > > > > > > > > > > >> > > > > > > > > > > > > > > >> > > > 11. Regarding the difference between the token > > > > bucket > > > > > > > > > algorithm > > > > > > > > > > > and > > > > > > > > > > > >> our > > > > > > > > > > > >> > > > current quota mechanism. That's a good > > > observation. > > > > It > > > > > > > seems > > > > > > > > > > that > > > > > > > > > > > we > > > > > > > > > > > >> > can > > > > > > > > > > > >> > > > make a slight change to our current quota > > > mechanism > > > > to > > > > > > > > > achieve a > > > > > > > > > > > >> > similar > > > > > > > > > > > >> > > > thing. As you said, the issue was that we > > allocate > > > > all > > > > > > 7 * > > > > > > > > 80 > > > > > > > > > > > >> requests > > > > > > > > > > > >> > in > > > > > > > > > > > >> > > > the last 1 sec window in our current > mechanism. > > > This > > > > > is > > > > > > a > > > > > > > > bit > > > > > > > > > > > >> > unintuitive > > > > > > > > > > > >> > > > since each sec only has a quota capacity of 5. > > An > > > > > > > > alternative > > > > > > > > > > way > > > > > > > > > > > >> is to > > > > > > > > > > > >> > > > allocate the 7 * 80 requests to all previous > > > > windows, > > > > > > each > > > > > > > > up > > > > > > > > > to > > > > > > > > > > > the > > > > > > > > > > > >> > > > remaining quota capacity left. So, you will > fill > > > the > > > > > > > > current 1 > > > > > > > > > > sec > > > > > > > > > > > >> > window > > > > > > > > > > > >> > > > and all previous ones, each with 5. Then, it > > seems > > > > > this > > > > > > > will > > > > > > > > > > give > > > > > > > > > > > >> the > > > > > > > > > > > >> > > same > > > > > > > > > > > >> > > > behavior as token bucket? The reason that I > keep > > > > > asking > > > > > > > this > > > > > > > > > is > > > > > > > > > > > that > > > > > > > > > > > >> > from > > > > > > > > > > > >> > > > an operational perspective, it's simpler if > all > > > > types > > > > > of > > > > > > > > > quotas > > > > > > > > > > > >> work in > > > > > > > > > > > >> > > the > > > > > > > > > > > >> > > > same way. > > > > > > > > > > > >> > > > > > > > > > > > > > > >> > > > Jun > > > > > > > > > > > >> > > > > > > > > > > > > > > >> > > > On Tue, May 26, 2020 at 9:52 AM David Jacot < > > > > > > > > > > dja...@confluent.io> > > > > > > > > > > > >> > wrote: > > > > > > > > > > > >> > > > > > > > > > > > > > > >> > > > > Hi folks, > > > > > > > > > > > >> > > > > > > > > > > > > > > > >> > > > > I have updated the KIP. As mentioned by > Jun, I > > > > have > > > > > > made > > > > > > > > the > > > > > > > > > > > >> > > > > quota per principal/clientid similarly to > the > > > > other > > > > > > > > quotas. > > > > > > > > > I > > > > > > > > > > > have > > > > > > > > > > > >> > > > > also explained how this will work in > > conjunction > > > > > with > > > > > > > the > > > > > > > > > auto > > > > > > > > > > > >> > > > > topics creation. > > > > > > > > > > > >> > > > > > > > > > > > > > > > >> > > > > Please, take a look at it. I plan to call a > > vote > > > > for > > > > > > it > > > > > > > in > > > > > > > > > the > > > > > > > > > > > >> next > > > > > > > > > > > >> > few > > > > > > > > > > > >> > > > > days if there are no comments in this > thread. > > > > > > > > > > > >> > > > > > > > > > > > > > > > >> > > > > Best, > > > > > > > > > > > >> > > > > David > > > > > > > > > > > >> > > > > > > > > > > > > > > > >> > > > > On Wed, May 13, 2020 at 10:57 AM Tom > Bentley < > > > > > > > > > > > tbent...@redhat.com > > > > > > > > > > > >> > > > > > > > > > > > > >> > > > wrote: > > > > > > > > > > > >> > > > > > > > > > > > > > > > >> > > > > > Hi David, > > > > > > > > > > > >> > > > > > > > > > > > > > > > > >> > > > > > Thanks for the explanation and > confirmation > > > that > > > > > > > > evolving > > > > > > > > > > the > > > > > > > > > > > >> APIs > > > > > > > > > > > >> > is > > > > > > > > > > > >> > > > not > > > > > > > > > > > >> > > > > > off the table in the longer term. > > > > > > > > > > > >> > > > > > > > > > > > > > > > > >> > > > > > Kind regards, > > > > > > > > > > > >> > > > > > > > > > > > > > > > > >> > > > > > Tom > > > > > > > > > > > >> > > > > > > > > > > > > > > > > >> > > > > > > > > > > > > > > > >> > > > > > > > > > > > > > > >> > > > > > > > > > > > > > >> > > > > > > > > > > > > >> > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
