search

skroutz / rafka

Kafka proxy with a simple API, speaking the Redis protocol
https://engineering.skroutz.gr/blog/kafka-rails-integration/
GNU General Public License v3.0
8 stars 0 forks source link

Drop the consumer_manager module completely #84

Closed dblia closed 4 years ago

dblia commented 5 years ago

This PR implements the Redefine Rafka scope and Drop redundant functionality proposals of the Rafka Rethinking design doc. For more details on the topic, please refer to those references.

A lot of major changes are introduced by this PR which are detailed in the respective commit messages. The most important ones are that the Client struct becomes aware of the server's Context as well as the Rafka configuration file. Since Client becomes the authoritative handler of Consumer objects, those two extra fields are essential to properly register and de-register a Consumer instance via the Client object.

Moreover, the consumer field is also added to the Client, which is a reference to the registered Consumer object. Also, a new function is introduced in client module, named registerConsumer which will be used to create a new Consumer instance upon request. The Consumer struct is extended with a new field containing the parent's cancel context (aka Client), which will allow us signal a Consumer instance about the Client's termination; a task which was performed until now by the ConsumerManager handler.

ctrochalakis commented 5 years ago

During rafka shutdown sequence, we waited for consumers to close gracefully (using the manager wg). This is now done in a non-blocking way and might result in non-committed offsets or other unexpected behavior.

ctrochalakis commented 5 years ago

Other that that, great work :) I think that practice has shown that having a single consumer per client is enough with the added benefit of a simpler overall design.

dblia commented 5 years ago

During rafka shutdown sequence, we waited for consumers to close gracefully (using the manager wg). This is now done in a non-blocking way and might result in non-committed offsets or other unexpected behavior.

Indeed, the Server's shutdown flow has been modified while it shouldn't. I'll restore the original behavior shortly. Nice catch, thank you!

dblia commented 5 years ago

There's an non-handled case in server: Revisit shutdown flow commit, which will be addresses shortly.

dblia commented 4 years ago

Squashed all commits and forced-push for a last look.

agis commented 4 years ago

:rocket: :clap: :tada: