Hi, David, Thanks for making those changes. They look fine to me. +1
Jun On Thu, Jun 11, 2020 at 11:51 PM David Jacot <dja...@confluent.io> wrote: > Colin, Jun, > > Do the proposed error code and the updated KIP look good to you guys? I’d > like to wrap up and close the vote. > > Thanks, > David > > Le mer. 10 juin 2020 à 14:50, David Jacot <dja...@confluent.io> a écrit : > > > Hi Colin and Jun, > > > > I have no problem if we have to rewrite part of it when the new > controller > > comes > > out. I will be more than happy to help out. > > > > Regarding KIP-590, I think that we can cope with a principal as a string > > when the > > time comes. The user entity name is defined with a string already. > > > > Regarding the name of the error, you have made a good point. I do agree > > that it > > is important to differentiate the two cases. I propose the following two > > errors: > > - THROTTLING_QUOTA_EXCEEDED - Throttling is slightly better than rate as > > we have quotas which are not rate (e.g. request quota). This one is > > retryable > > once the throttle time is passed. > > - LIMIT_QUOTA_EXCEEDED - This one would indicate that the limit has been > > reached and is a final error. > > We only need the former in this KIP. What do you think? > > > > Jun, I have added a few examples in the KIP. The new name works exactly > > like > > the existing one once it is added to the accepted dynamic configs for the > > user > > and the client entities. I have added a "Kafka Config Command" chapter in > > the > > KIP. I will also open a Jira to not forget updating the AK documentation > > once > > the KIP gets merged. > > > > Thanks, > > David > > > > On Wed, Jun 10, 2020 at 3:03 AM Jun Rao <j...@confluent.io> wrote: > > > >> Hi, Colin, > >> > >> Good point. Maybe sth like THROTTLING_QUOTA_VIOLATED will make this > clear. > >> > >> Hi, David, > >> > >> We added a new quota name in the KIP. You chose not to bump up the > version > >> of DESCRIBE_CLIENT_QUOTAS and ALTER_CLIENT_QUOTAS, which seems ok since > >> the > >> quota name is represented as a string. However, the new quota name can > be > >> used in client tools for setting and listing the quota ( > >> https://kafka.apache.org/documentation/#quotas). Could you document how > >> the > >> new name will be used in those tools? > >> > >> Thanks, > >> > >> Jun > >> > >> On Tue, Jun 9, 2020 at 3:37 PM Colin McCabe <cmcc...@apache.org> wrote: > >> > >> > On Tue, Jun 9, 2020, at 05:06, David Jacot 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 > >> > > 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. > >> > > > >> > > >> > Hi David, > >> > > >> > Good question. The approach we are thinking of is that in the future, > >> > topic creation will be a controller RPC. In other words, rather than > >> > changing ZK and waiting for the controller code to notice, we'll go > >> through > >> > the controller code (which may change ZK, or may do something else in > >> the > >> > ZK-free environment). > >> > > >> > I don't think there is a good way to write this in the short term that > >> > avoids having to rewrite in the long term. That's probably OK though, > >> as > >> > long as we keep in mind that we'll need to. > >> > > >> > > > >> > > 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. > >> > > > >> > > >> > Right. The main requirement here is that the quota must operate based > >> on > >> > principal names rather than KafkaPrincipal objects. We had a long > >> > discussion about KIP-590 and eventually concluded that we didn't want > to > >> > make KafkaPrincipal serializable (at least not yet) so what would be > >> > forwarded is just a string, not the principal object. > >> > > >> > > > >> > > 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. > >> > > > >> > > >> > Think about if you're someone writing software that uses the Kafka > >> > client. Let's say you try to create some topics and get back > >> > QUOTA_VIOLATED. What does it mean? Maybe it means that you tried to > >> > create too many topics in too short a time (violating the controller > >> > throttle). Or maybe it means that you exceeded your quota specifying > >> the > >> > number of partitions that they are allowed to create (let's assume > that > >> > someone did a follow-on KIP for that that reuses this error code for > >> that.) > >> > > >> > But now you have a dilemma. If the controller was just busy, then > >> trying > >> > again is the right thing to do. If there is some other quota you > >> violated > >> > (number of partitions, number of topics, etc.) then retrying is > wasteful > >> > and will not accomplish anything. Of course, the Kafka client > software > >> > itself can tell the two cases apart since it has access to > >> throttleTimeMs. > >> > But the end user can't (and in certain modes, the exception is exposed > >> > directly to the user here). > >> > > >> > That's why "you violated some quota, not sure which" is not a good > error > >> > code. So I think we should distinguish the two cases by having two > >> > separate error codes. Maybe something like RATE_QUOTA_VIOLATED and > >> > RESOURCE_QUOTA_VIOLATED. This KIP would only need the former one. > >> > > >> > Another possibility is that the user could distinguish the two cases > by > >> > the error string. But typically we want error strings to be used to > >> give > >> > extra debugging information, not to make big decisions about what the > >> > client should do. So I think that the error code should actually be a > >> > little bit more specific, or at least tell the end user what to do > with > >> it > >> > (that's why I suggested "busy"). > >> > > >> > > > >> > > Voilà. I hope that I have addressed all your questions/points and I > am > >> > > sorry for the lengthy email. > >> > > > >> > > >> > Thanks, David. It looks good to me overall. And I thought your email > >> was > >> > very clear-- not too long at all. > >> > Let's just figure out the error code thing. > >> > > >> > best, > >> > Colin > >> > > >> > > > >> > > 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 > >> > > > > > > > >> > > > > > > >> > > > > > >> > > > > >> > > > >> > > >> > > >
