Drop the consumer_manager module completely #84
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
During |
|
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. |
Indeed, the Server's shutdown flow has been modified while it shouldn't. I'll restore the original behavior shortly. Nice catch, thank you! |
agis
reviewed
Sep 24, 2019
|
There's an non-handled case in |
agis
reviewed
Oct 1, 2019
0xdiba
reviewed
Oct 2, 2019
Currently, a `Client` instance is allowed to have more than one `Consumer` instances associated with it. As the respective ddoc details (`design-rafka-rethinking.rst`), there is no actual reason to maintain this one-to-many relationship between Clients and Consumers which among others corresponds to a lot of extra logic in the source code. This commit drops support for multiple `Consumer` instances per `Client`; from now on, a `Client` instance can only be mapped to a single `Consumer`. In order to drop this functionality, the following modifications have been applied: - drop the `Client.consumers` map completely - drop the `Client.consByTopic` map completely - (temporarily) introduce the `consID` field on Client struct - replace the `ConsumerByTopic` function with the `fetchConsumer` equivalent As mentioned, the `consID` field is temporary; it is mainly used to facilitate the transition to a `ConsumerManager`-free Rafka server.
The `ConsumerID` type semantically belongs to the consumer module. This commit moves the definition from `consumer_manager.go` to `consumer.go` module, and also casts the `Consumer.id` field to the `ConsumerID` type, as it should be.
This commit introduces some major changes in Rafka internals, as the `design-rafka-rethinking.rst` ddoc details. In short, the `consumer_manager.go` module and all the respective logic, is completely dropped from the source code. Subsequently, the handling of the `Consumer` instances will be shared between the `Client` and `Server` instances. To do so, we've had to make the `Client` struct 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 temporary `consID` Client's field is removed and replaced by the `consumer` one, which is a reference to the `Client`'s `Consumer` object. Also, a new function is introduced in `Client`, 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.
When the server is requested to shutdown we need to wait for all active consumers to close prior to stopping the server, a task which was among ConsumerManager's responsibilities. Now that the respective module is dropped, we make the server the authoritative controller of this task via a `sync.WaitGroup` struct.
A server shutdown operation may last longer than expected in case the Rafka server is loaded. During this interval we don't want to register any new Consumers to Rafka. This commit introduces a new flag in server struct named `teardown`, which denotes whether a shutdown request is in-progress. If so, no new consumers will be registered and an appropriate message will be returned to the client. Since the `teardown` flag is shared among the Server and its Client's, we had to guard its references using a `sync.RWMUtex`. Note: In contrast to Consumer's handling, we allow new Producers to be registered while the server shuts down. Producers are way more lightweight comparing to Consumers, and so we can start and stop them freely. This way we, we also descrease the downtime interval of registered Producers in Rafka.
This new folder will contain all Rafka-specific documents. The already existing `ddocs` folder as well as its contents, are moved under the `docs/designs` subdirectory.
Add a document detailing the server's shutdown flow and describing what should be expected during this process.
|
Squashed all commits and forced-push for a last look. |
0xdiba
approved these changes
Oct 4, 2019
agis
approved these changes
Oct 7, 2019
|
🚀 👏 🎉 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Clientstruct becomes aware of the server'sContextas well as the Rafka configuration file. SinceClientbecomes the authoritative handler ofConsumerobjects, those two extra fields are essential to properly register and de-register aConsumerinstance via theClientobject.Moreover, the
consumerfield is also added to theClient, which is a reference to the registeredConsumerobject. Also, a new function is introduced inclientmodule, namedregisterConsumerwhich will be used to create a newConsumerinstance upon request. TheConsumerstruct is extended with a new field containing the parent's cancel context (akaClient), which will allow us signal aConsumerinstance about the Client's termination; a task which was performed until now by theConsumerManagerhandler.