Blizzard / node-rdkafka

Node.js bindings for librdkafka
MIT License
2.08k stars 391 forks source link

feat: add global consume timeout #1061

Open bojan-rajkovic-simplisafe opened 7 months ago

bojan-rajkovic-simplisafe commented 7 months ago

This solves a similar problem to #1052, but does it with an approach that I think is a little cleaner and doesn't change existing consumer behavior — even if the existing consumer behavior is arguably wrong, all behavioral changes are susceptible to the spacebar heating problem.

Rather than making the specified timeout both the single-message consume timeout and the batch timeout, it introduces a new value called the total consume timeout, which controls how long we can spend trying to fill a batch of n messages with the consume(num, cb) variant, and checks it on every iteration of the consume loop.

I did borrow the tests from that PR though, as they provided a pretty elegant framework to check the approach.

bojan-rajkovic-simplisafe commented 7 months ago

e2e is failing, appears to be related to the argument checking — I suspect I either didn’t rebuild the integration fully or messed up a check somewhere…

bojan-rajkovic-simplisafe commented 7 months ago

Fixed — in the process, ended up cleaning up C++-land a bit and killing some dead code + some code that I'm not sure I understand how it ever worked. :)

bojan-rajkovic-simplisafe commented 7 months ago

@GaryWilber @iradul It might be good to get one of you to look at both this and #1053 and decide which approach to take, merge one, and do some testing before cutting a release. I'm fairly confident in mine, but it could probably use more testing regardless…