mfontanini / cppkafka

Modern C++ Apache Kafka client library (wrapper for librdkafka)
BSD 2-Clause "Simplified" License
600 stars 206 forks source link

Added clarifications and comments to the BufferedProducer class #237

Closed accelerated closed 4 years ago

accelerated commented 4 years ago

Added more comments and clarifications for the BufferedProducer class. There are also small changes namely:

accelerated commented 4 years ago

Not sure why one of the sync producer tests fails intermittently on Travis. I ran it in a loop on latest 1.3.0 rdkafka version and i never got it to fail. Made some cmake changes to allow easier test setup in other environments outside of Travis.

accelerated commented 4 years ago

Ok this should be good now. I figured out the intermittent failing. Essentially, when calling sync_flush with a timeout, the first change I made was calling wait_for_acks(timeout) right before waiting on the future. There was a race condition which happened sometimes when wait_for_acks would actually time out, it would then block on the future in the while loop iteration. Because the main thread was now blocked, the queue was no longer flushed, which meant that the delivery callback was never called, thus the future would never return. I never saw this in my testing because we always poll/flush on a separate thread so this blocking never happened. In the Catch2 test suite, the setup is simpler so all the polling and flushing is done by the same main thread.

mfontanini commented 4 years ago

Wow this is so much code. I'll hold off until I'm more awake, it's 7 am here :).

accelerated commented 4 years ago

Wow this is so much code. I'll hold off until I'm more awake, it's 7 am here :).

Sure! I had to cleanup this class and also fixed a race-condition + some performance problems.

mfontanini commented 4 years ago

Well I dunno, I can't really follow what's happening in this class anymore without sitting for a few hours on it. I presume you've tested this and it works fine for you so I'll trust that it works.

Sorry for the delay on this!

accelerated commented 4 years ago

@mfontanini thanks! it's on beta at the moment and no issues. Comparing the diff is a bit hard, I would recommend just reading the code as-is. It's shorter now and simpler so it should be easy to follow (hopefully). In any case, if any bugs arise, i'm very committed to fixing them asap.