Address issues in InMemorySubscriber#141
Merged
stevenhartley merged 5 commits intomainfrom Jul 10, 2024
Merged
Conversation
The subscribe API is not idempotent and we fixed unsubscribe() so that there is no issue where we unsubscribe but an handler or listener stays registered. #140
|
Code coverage report is ready! 📈
|
neelam-kushwah
requested changes
Jul 8, 2024
src/main/java/org/eclipse/uprotocol/communication/InMemorySubscriber.java
Outdated
Show resolved
Hide resolved
neelam-kushwah
approved these changes
Jul 8, 2024
sophokles73
reviewed
Jul 9, 2024
src/main/java/org/eclipse/uprotocol/communication/InMemorySubscriber.java
Outdated
Show resolved
Hide resolved
src/main/java/org/eclipse/uprotocol/communication/InMemorySubscriber.java
Outdated
Show resolved
Hide resolved
sophokles73
reviewed
Jul 9, 2024
src/main/java/org/eclipse/uprotocol/communication/InMemorySubscriber.java
Outdated
Show resolved
Hide resolved
|
Code coverage report is ready! 📈
|
|
Code coverage report is ready! 📈
|
src/main/java/org/eclipse/uprotocol/communication/InMemorySubscriber.java
Outdated
Show resolved
Hide resolved
sophokles73
reviewed
Jul 10, 2024
Contributor
sophokles73
left a comment
sophokles73
left a comment
There was a problem hiding this comment.
When in line 130 of InMemorySuscriber the transport.registerListener(topic, listener) call fails, then we have successfully (logically) subscribed to the topic (using USubscription) but have failed to register the listener and return an exception back to the client.
IMHO this is not a real problem as it only means that we might start forwarding events to the local authority which are not being consumed. It might be worth adding a corresponding comment to the code so that anybody reading it knows that we are aware of this and considered it unproblematic.
dddj698
reviewed
Jul 10, 2024
src/main/java/org/eclipse/uprotocol/communication/InMemorySubscriber.java
Outdated
Show resolved
Hide resolved
we will remove the handler regardless if the unregisterListener is successful or not and when we call unsubscribe or we call unregisterListener() API. Also keeping support for the handler to be used for other state transitions.
|
Code coverage report is ready! 📈
|
|
Code coverage report is ready! 📈
|
dddj698
approved these changes
Jul 10, 2024
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
4 participants
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.
The subscribe API is now idempotent and we fixed unsubscribe() so that there is no issue where we unsubscribe but an handler or listener stays registered.
#140