Bump API versions for client-side throttling #933
Conversation
…c.js files into the requests test The metadata check is required for all request implementations, and having it separate in each test just increases the chance for not having it everywhere ... which is exactly the situation right now. This commit _increases_ test coverage therefore, by now making sure the test is run for each request.
…ly supported APIs This commit accomplishes a few things: 1. Ensure that iff `clientSideThrottleTime` is set, `throttleTime` is set to 0. This was the original intent of the commit adding support for client-side throttling, so that it is very obvious who did the throttling. 2. Add the missing API versions for the client-side throttling bump in a single commit, rather than hoping that they appear over time. This allows to align the code for these to be a version of "decode the response with the previous version, and then move the throttleTime to clientSideThrottleTime". 3. Update the tests to consider `clientSideThrottleTime` optionally, so that the tests should pass on quite a few previous Kafka versions.
|
|
||
| return { | ||
| ...decoded, | ||
| throttleTime: 0, |
There was a problem hiding this comment.
Why is it that you're overriding throttleTime here?
I can see the logic that you want to say that the request wasn't throttled, and that we are supposed to throttle on our side, but it's inconsistent with what we're doing everywhere else, where we return the original value for throttleTime and simply copy it to clientSideThrottleTime.
There was a problem hiding this comment.
That said, I think I like your decision. I just wanna make sure we're consistent.
There was a problem hiding this comment.
I think I actually changed all places to set throttleTime to 0 when returning a client-side value. :)
The thinking is that "throttleTime" greater than 0 means the broker throttled for that amount; while "clientSideThrottleTime" greater 0 indicates that the client did/should do so.
|
Thanks for taking on this unbelievably boring task. I had been meaning to do it for a while now, but I just never could bring myself to it. 😅 |
This is a fairly big, but rather mechanical piece of work: Review the existing requests and ensure that the bump for https://cwiki.apache.org/confluence/display/KAFKA/KIP-219+-+Improve+quota+communication is done (i.e. the throttleTime is reported as clientSideThrottleTime), and for all APIs used by KafkaJS that didn't support the client-side throttling yet add the needed API versions.
The first commit of the PR ensures that when adding the new APIs I didn't accidentally miss a bump somewhere, by moving the 'metadata about the API' test from the individual request.spec.js files into src/prototol/requests/index.spec.js, and making it generic over all requests defined.