kafkajs
kafkajs copied to clipboard
tulios
fix regression when throttle timeout is marginal
Fixes https://github.com/tulios/kafkajs/issues/1556
Tries to target the regression that was caused by the mentioned issue. The change to check whether the throttleTimeout is actually greater than 0 is readded. To cover for cases the result of the timeUntilUnthrottled calculation was actually off by a small margin might be fixed by storing the timestamp when the throttleing was applied. And after the check runs, updating it. Please let me now if i missed something as this is my first draft for this. The test that was added works with this but it might still be lacking something.
@Nevon I would greatly appreciate it if you could review this pull request once again and ultimately approve and merge it. We are currently facing errors that have already been fixed in version 2.2.4, but unfortunately, that version is not currently usable. Thank you!
Was very glad to find this specific issue having been reported and a fix proposed for it!
Ought there to be a test that would guard against this kind of performance degradation to happen in the future?
How we ended up bumping into this was that there was an integration test that did time jumps with Sinon FakeTimer where it was clear that the internals of FakeTimer ended up throttling the actual Event Loop due to this setTimeout (and another setInterval) causing there to be a huge backlog of timer callbacks. Not saying that this is the strategy that one ought to use to guard against this, but that would indirectly create somewhat easy way of verifying that there are no overly-aggressive setTimeout-zero loops in the system.
As this has been open since May we have decided to abandon hope and downgrade to v1.16 to not have these issues. :(
This is really sad, we had to upgrade to KafkaJS and all the problems related tu CPU usage started... we were using KafkaJS 1.6.0 and no problem... this fix WORKS why so many months with out doing anything? Basically KafkaJS 2 is just bad.
Be aware of updating to KafkaJS 2.x.x
i feel like this PR was used to vent a lot, however in order to contribute, i think it makes sense to move this forward and release it as a patch version
Feel free to try out installing: https://github.com/MDSLKTR/kafkajs.git#2.2.5, in case that helps your case, no guarantees though obviously
@MDSLKTR thank a lot for your work! How soon can we expect this change in the official release? It seems to me that the previous reviewer (@Nevon) has gone inactive in this thread. @ekanth may you help somehow to push this through?
Our team will be switching back to node-rdkafka while keeping in mind that the official client will be ready at some point https://github.com/confluentinc/confluent-kafka-javascript So i wont pursue this any further