Hi, David, Sounds good then.
Thanks, Jun On Tue, Jun 9, 2020 at 10:59 AM David Jacot <dja...@confluent.io> wrote: > Hi Jun, > > Both are already in the KIP, see "New Broker Configurations" chapter. I > think > that we need them in order to be able to define different burst for the new > quota. > > Best, > David > > On Tue, Jun 9, 2020 at 7:48 PM Jun Rao <j...@confluent.io> wrote: > > > Hi, David, > > > > Another thing. Should we add controller.quota.window.size.seconds and > > controller.quota.window.num > > or just reuse the existing quota.window.size.seconds and quota.window.num > > that are used for other types of quotas? > > > > Thanks, > > > > Jun > > > > On Tue, Jun 9, 2020 at 10:30 AM Jun Rao <j...@confluent.io> wrote: > > > > > Hi, David, > > > > > > Thanks for the KIP. The name QUOTA_VIOLATED sounds reasonable to me. +1 > > on > > > the KIP. > > > > > > Jun > > > > > > On Tue, Jun 9, 2020 at 5:07 AM David Jacot <dja...@confluent.io> > wrote: > > > > > >> Hi Colin, > > >> > > >> Thank you for your feedback. > > >> > > >> Jun has summarized the situation pretty well. Thanks Jun! I would like > > to > > >> complement it with the following points: > > >> > > >> 1. Indeed, when the quota is exceeded, the broker will reject the > topic > > >> creations, partition creations and topics deletions that are exceeding > > >> with the new QUOTA_VIOLATED error. The ThrottleTimeMs field will > > >> be populated accordingly to let the client know how long it must wait. > > >> > > >> 2. I do agree that we actually want a mechanism to apply back pressure > > >> to the clients. The KIP basically proposes a mechanism to control and > to > > >> limit the rate of operations before entering the controller. I think > > that > > >> it is > > >> similar to your thinking but is enforced based on a defined quota > > instead > > >> of relying on the number of pending items in the controller. > > >> > > >> 3. You mentioned an alternative idea in your comments that, if I > > >> understood > > >> correctly, would bound the queue to limit the overload and reject work > > if > > >> the > > >> queue is full. I have been thinking about this as well but I don't > think > > >> that it > > >> works well in our case. > > >> - The first reason is the one mentioned by Jun. We actually want to be > > >> able > > >> to limit specific clients (the misbehaving ones) in a multi-tenant > > >> environment. > > >> - The second reason is that, at least in our current implementation, > the > > >> length of > > >> the queue is not really a good characteristic to estimate the load. > > >> Coming back > > >> to your example of the CreateTopicsRequest. They create path in ZK for > > >> each > > >> newly created topics which trigger a ChangeTopic event in the > > controller. > > >> That > > >> single event could be for a single topic in some cases or for a > thousand > > >> topics > > >> in others. > > >> These two reasons aside, bounding the queue also introduces a knob > which > > >> requires some tuning and thus suffers from all the points you > mentioned > > as > > >> well, isn't it? The value may be true for one version but not for > > another. > > >> > > >> 4. Regarding the integration with KIP-500. The implementation of this > > KIP > > >> will span across the ApiLayer and the AdminManager. To be honest, we > > >> can influence the implementation to work best with what you have in > mind > > >> for the future controller. If you could shed some more light on the > > future > > >> internal architecture, I can take this into account during the > > >> implementation. > > >> > > >> 5. Regarding KIP-590. For the create topics request, create partitions > > >> request, > > >> and delete topics request, we are lucky enough to have directed all of > > >> them > > >> to > > >> the controller already so we are fine with doing the admission in the > > >> broker > > >> which hosts the controller. If we were to throttle more operations in > > the > > >> future, > > >> I believe that we can continue to do it centrally while leveraging the > > >> principal > > >> that is forwarded to account for the right tenant. The reason why I > > would > > >> like > > >> to keep it central is that we are talking about sparse (or bursty) > > >> workloads here > > >> so distributing the quota among all the brokers makes little sense. > > >> > > >> 6. Regarding the naming of the new error code. BUSY sounds too generic > > to > > >> me so I would rather prefer a specific error code. The main reason > being > > >> that > > >> we may be able to reuse it in the future for other quotas. That being > > >> said, > > >> we > > >> can find another name. QUOTA_EXHAUSTED? I don't feel too strongly > about > > >> the naming. I wonder what the others think about this. > > >> > > >> Voilà. I hope that I have addressed all your questions/points and I am > > >> sorry for > > >> the lengthy email. > > >> > > >> Regards, > > >> David > > >> > > >> > > >> On Tue, Jun 9, 2020 at 2:13 AM Colin McCabe <cmcc...@apache.org> > wrote: > > >> > > >> > On Mon, Jun 8, 2020, at 14:41, Jun Rao wrote: > > >> > > Hi, Colin, > > >> > > > > >> > > Thanks for the comment. You brought up several points. > > >> > > > > >> > > 1. Should we set up a per user quota? To me, it does seem we need > > some > > >> > sort > > >> > > of a quota. When the controller runs out of resources, ideally, we > > >> only > > >> > > want to penalize the bad behaving applications, instead of every > > >> > > application. To do that, we will need to know what each > application > > is > > >> > > entitled to and the per user quota is intended to capture that. > > >> > > > > >> > > 2. How easy is it to configure a quota? The following is how an > > admin > > >> > > typically sets up a quota in our existing systems. Pick a generous > > >> > default > > >> > > per user quota works for most applications. For the few resource > > >> > intensive > > >> > > applications, customize a higher quota for them. Reserve enough > > >> resources > > >> > > in anticipation that a single (or a few) application will exceed > the > > >> > quota > > >> > > at a given time. > > >> > > > > >> > > > >> > Hi Jun, > > >> > > > >> > Thanks for the response. > > >> > > > >> > Maybe I was too pessimistic about the ability of admins to > configure a > > >> > useful quota here. I do agree that it would be nice to have the > > >> ability to > > >> > set different quotas for different users, as you mentioned. > > >> > > > >> > > > > >> > > 3. How should the quota be defined? In the discussion thread, we > > >> debated > > >> > > between a usage based model vs a rate based model. Dave and Anna > > >> argued > > >> > for > > >> > > the rate based model mostly because it's simpler to implement. > > >> > > > > >> > > > >> > I'm trying to think more about how this integrates with our plans > for > > >> > KIP-500. When we get rid of ZK, we will have to handle this in the > > >> > controller itself, rather than in the AdminManager. That implies > > we'll > > >> > have to rewrite the code. Maybe this is worth it if we want this > > >> feature > > >> > now, though. > > >> > > > >> > Another wrinkle here is that as we discussed in KIP-590, controller > > >> > operations will land on a random broker first, and only then be > > >> forwarded > > >> > to the active controller. This implies that either admissions > control > > >> > should happen on all brokers (needing some kind of distributed quota > > >> > scheme), or be done on the controller after we've already done the > > work > > >> of > > >> > forwarding the message. The second approach might not be that bad, > > but > > >> it > > >> > would be nice to figure this out. > > >> > > > >> > > > > >> > > 4. If a quota is exceeded, how is that enforced? My understanding > of > > >> the > > >> > > KIP is that, if a quota is exceeded, the broker immediately sends > > back > > >> > > a QUOTA_VIOLATED error and a throttle time back to the client, and > > the > > >> > > client will wait for the throttle time before issuing the next > > >> request. > > >> > > This seems to be the same as the BUSY error code you mentioned. > > >> > > > > >> > > > >> > Yes, I agree, it sounds like we're thinking along the same lines. > > >> > However, rather than QUOTA_VIOLATED, how about naming the error code > > >> BUSY? > > >> > Then the error text could indicate the quota that we violated. This > > >> would > > >> > be more generally useful as an error code and also avoid being > > >> confusingly > > >> > similar to POLICY_VIOLATION. > > >> > > > >> > best, > > >> > Colin > > >> > > > >> > > > > >> > > I will let David chime in more on that. > > >> > > > > >> > > Thanks, > > >> > > > > >> > > Jun > > >> > > > > >> > > > > >> > > > > >> > > On Sun, Jun 7, 2020 at 2:30 PM Colin McCabe <cmcc...@apache.org> > > >> wrote: > > >> > > > > >> > > > Hi David, > > >> > > > > > >> > > > Thanks for the KIP. > > >> > > > > > >> > > > I thought about this for a while and I actually think this > > approach > > >> is > > >> > not > > >> > > > quite right. The problem that I see here is that using an > > >> explicitly > > >> > set > > >> > > > quota here requires careful tuning by the cluster operator. > Even > > >> > worse, > > >> > > > this tuning might be invalidated by changes in overall > conditions > > or > > >> > even > > >> > > > more efficient controller software. > > >> > > > > > >> > > > For example, if we empirically find that the controller can do > > 1000 > > >> > topics > > >> > > > in a minute (or whatever), this tuning might actually be wrong > if > > >> the > > >> > next > > >> > > > version of the software can do 2000 topics in a minute because > of > > >> > > > efficiency upgrades. Or, the broker that the controller is > > located > > >> on > > >> > > > might be experiencing heavy load from its non-controller > > operations, > > >> > and so > > >> > > > it can only do 500 topics in a minute during this period. > > >> > > > > > >> > > > So the system administrator gets a very obscure tunable (it's > not > > >> > clear to > > >> > > > a non-Kafka-developer what "controller mutations" are or why > they > > >> > should > > >> > > > care). And even worse, they will have to significantly > "sandbag" > > >> the > > >> > value > > >> > > > that they set it to, so that even under the heaviest load and > > oldest > > >> > > > deployed version of the software, the controller can still > > function. > > >> > Even > > >> > > > worse, this new quota adds a lot of complexity to the > controller. > > >> > > > > > >> > > > What we really want is backpressure when the controller is > > >> > overloaded. I > > >> > > > believe this is the alternative you discuss in "Rejected > > >> Alternatives" > > >> > > > under "Throttle the Execution instead of the Admission" Your > > reason > > >> > for > > >> > > > rejecting it is that the client error handling does not work > well > > in > > >> > this > > >> > > > case. But actually, this is an artifact of our current > > >> implementation, > > >> > > > rather than a fundamental issue with backpressure. > > >> > > > > > >> > > > Consider the example of a CreateTopicsRequest. The controller > > could > > >> > > > return a special error code if the load was too high, and take > the > > >> > create > > >> > > > topics event off the controller queue. Let's call that error > code > > >> > BUSY. > > >> > > > Additionally, the controller could immediately refuse new > events > > if > > >> > the > > >> > > > queue had reached its maximum length, and simply return BUSY for > > >> that > > >> > case > > >> > > > as well. > > >> > > > > > >> > > > Basically, the way we handle RPC timeouts in the controller > right > > >> now > > >> > is > > >> > > > not very good. As you know, we time out the RPC, so the client > > gets > > >> > > > TimeoutException, but then keep the event on the queue, so that > it > > >> > > > eventually gets executed! There's no reason why we have to do > > that. > > >> > We > > >> > > > could take the event off the queue if there is a timeout. This > > >> would > > >> > > > reduce load and mostly avoid the paradoxical situations you > > describe > > >> > > > (getting TopicExistsException for a CreateTopicsRequest retry, > > etc.) > > >> > > > > > >> > > > I say "mostly" because there are still cases where retries could > > go > > >> > astray > > >> > > > (for example if we execute the topic creation but a network > > problem > > >> > > > prevents the response from being sent to the client). But this > > >> would > > >> > still > > >> > > > be a very big improvement over what we have now. > > >> > > > > > >> > > > Sorry for commenting so late on this but I got distracted by > some > > >> other > > >> > > > things. I hope we can figure this one out-- I feel like there > is > > a > > >> > chance > > >> > > > to significantly simplify this. > > >> > > > > > >> > > > best, > > >> > > > Colin > > >> > > > > > >> > > > > > >> > > > On Fri, May 29, 2020, at 07:57, David Jacot wrote: > > >> > > > > Hi folks, > > >> > > > > > > >> > > > > I'd like to start the vote for KIP-599 which proposes a new > > quota > > >> to > > >> > > > > throttle create topic, create partition, and delete topics > > >> > operations to > > >> > > > > protect the Kafka controller: > > >> > > > > > > >> > > > > > >> > > > >> > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-599%3A+Throttle+Create+Topic%2C+Create+Partition+and+Delete+Topic+Operations > > >> > > > > > > >> > > > > Please, let me know what you think. > > >> > > > > > > >> > > > > Cheers, > > >> > > > > David > > >> > > > > > > >> > > > > > >> > > > > >> > > > >> > > > > > >
