Poll heartbeat rate - Githubissues

paualarco commented 3 years ago

The main motivations for this PR is to tackle two existing issues https://github.com/monix/monix-kafka/issues/94 and https://github.com/monix/monix-kafka/issues/101. Rebasing existing pending PR: https://github.com/monix/monix-kafka/pull/104, so credits for the author, @voidconductor 🙏

Proper commitAsync implementation that removed callback from methods arguments itself, now completed in this callback, and it was also never used actually used in previous implementations. No api changes.
Polling strategy for keeping the consumer alive and not trigger partition reassignment in slow subscribers, which avoids doing the hack of decreasing poll.max.records. https://github.com/monix/monix-kafka/issues/101. The polling rate is only configurable internally for performance analysis on benchmarks repos. Ideally I we would just This polling is achieved by performing poll(0) from a task running concurrently in the background, which in case of error it the schedule will report it, without stopping the subscription. Although it should never happen since the heartbeat poll is only called if the consumer is paused, and both processes are call synchronized.

paualarco commented 3 years ago

@Avasil thank you for all the comments! Will address them and do review again 🧐

paualarco commented 3 years ago

@Avasil first of all sorry for the amount of commits, it might make difficult to track the changes review, I assume they will be squashed at the end...

I have written specific new cases in PollHeartbeatTest for the different kafka versions (excluding 0.9.x), and benchmarks covering manual and auto-commit scenarios with different heartbeat interval values. As part of that I ran the same benchmarks from latest release RC7, to have a base to compare with, and from there I've got two conclusions:

The higher pollHeartBeatInterval is, the slower the consumption becomes (although I am unsure what this relation is due to). So I ended up setting by default a heartbeat interval of 15ms.
On the other hand, I have noticed that the auto-commit-async has degraded and I am not sure why (I'm investigating it).

Finally, I was also tempted to move from travis to github actions and splitting the tests by the different kafka versions.

Avasil commented 3 years ago

first of all sorry for the amount of commits, it might make difficult to track the changes review, I assume they will be squashed at the end...

No worries, just let me know when you think it's good to re-review - thanks for the hard work!

As part of that I ran the same benchmarks from latest release RC7, to have a base to compare with, and from there I've got two conclusions:

I'll look at the benchmarks later but I feel like they can be unreliable if everything is hosted on one machine so it's hard to draw any conclusions

Finally, I was also tempted to move from travis to github actions and splitting the tests by the different kafka versions.

👍 Yeah, it needs to be done and I could setup sbt ci-release

paualarco commented 3 years ago

@Avasil I think it could already be re-reviewed :)

paualarco commented 3 years ago

Would be nice to give it a try in a real project though.

That would be ideal, alternatively we may ask it in the main monix gitter channel too, and see if someone is interested in testing it.

monix / monix-kafka

Poll heartbeat rate #250