Refactor the producers list replace the reference with the id as map key#249
Merged
Gsantomaggio merged 14 commits intomasterfrom Oct 2, 2025
Merged
Refactor the producers list replace the reference with the id as map key#249Gsantomaggio merged 14 commits intomasterfrom
Gsantomaggio merged 14 commits intomasterfrom
Conversation
Signed-off-by: Gabriele Santomaggio <[email protected]>
Gsantomaggio
changed the title
This reverts commit 53ca444.
Signed-off-by: Gabriele Santomaggio <[email protected]>
Signed-off-by: Gabriele Santomaggio <[email protected]>
Signed-off-by: Gabriele Santomaggio <[email protected]>
Contributor
There was a problem hiding this comment.
Pull Request Overview
Refactors the producer implementation to use publisher IDs as map keys instead of reference strings, improving performance and consistency in publisher management.
- Replaces string references with integer IDs as the primary key for tracking publishers
- Updates handler registration to use publisher IDs instead of references
- Modifies publisher creation logic to handle optional references
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| rstream/producer.py | Core refactoring to use publisher IDs as keys and handle optional references |
| rstream/client.py | Updates declare_publisher to accept optional reference parameter |
| rstream/consumer.py | Re-enables max consumers validation and adjusts subscription ID handling |
| tests/test_producer.py | Updates test calls to use named parameters and publisher names |
| tests/test_validate_id.py | Adds new test to validate publisher ID assignment and cleanup |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Signed-off-by: Gabriele Santomaggio <[email protected]>
Signed-off-by: Gabriele Santomaggio <[email protected]>
- it is possible to have only one stream for Consumer class - add cluster tests - use the publisher id to handle the publishers list - replace list of used ids with an atomic integer Signed-off-by: Gabriele Santomaggio <[email protected]>
Contributor
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 20 out of 21 changed files in this pull request and generated 3 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Comment on lines
+784
to
+785
| stream = self._publishers[publisher_id].stream | ||
| if publisher_id in self._publishers: |
There was a problem hiding this comment.
Potential KeyError if publisher_id is not in self._publishers. The stream assignment on line 784 should be moved inside the conditional check on line 785.
| await self._clients[stream].remove_stream(stream) | ||
| await self._clients[stream].free_available_id(subscriber.subscription_id) | ||
| await self._clients[stream].free_available_id() | ||
| del self._clients[stream] |
There was a problem hiding this comment.
The client is being deleted immediately after freeing the available ID, which may not be correct if other subscribers are using the same client. Consider checking if the client is still needed before deletion.
Suggested change
| del self._clients[stream] | |
| # Only delete the client if no other subscribers are using this stream | |
| if not any( | |
| s.stream == stream and sid != subscriber_id | |
| for sid, s in self._subscribers.items() | |
| ): | |
| del self._clients[stream] |
Co-authored-by: Copilot <[email protected]>
Signed-off-by: Gabriele Santomaggio <[email protected]>
Gsantomaggio
added
enhancement
This file contains hidden or 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
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.
Refactors the producer implementation to use publisher IDs as map keys instead of reference strings, improving performance and consistency in publisher management.
Breaking changes