-
Notifications
You must be signed in to change notification settings - Fork 5
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Introduce ResultStreamPublisher to interact with a ResultStream through a Publisher. #142
Introduce ResultStreamPublisher to interact with a ResultStream through a Publisher. #142
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few recommendations to simplify things.
src/test/java/io/axoniq/axonserver/connector/FakeResultStream.java
Outdated
Show resolved
Hide resolved
src/main/java/io/axoniq/axonserver/connector/ResultStreamPublisher.java
Outdated
Show resolved
Hide resolved
ResultStream<Message> resultStream) { | ||
this.subscriber = subscriber; | ||
this.resultStream = resultStream; | ||
this.resultStream.onAvailable(this::signal); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having callbacks triggered to an object that hasn't been constructed completely yet is generally not recommended.
In a comparable situation, we have chosen to register this callback in the request method, with the following pseudo-code:
if (requested.getAndUpdate(prev -> prev + n) == 0) {
this.resultStream.onAvailable(this::signal);
}
signal();
}
The resultstream will only remember the last registered callback, so there is only very little overhead in this operation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about moving that line into afterSubscribe method?
private void afterSubscribe() {
this.resultStream.onAvailable(this::signal);
signal();
}
In this way, it is executed only once.
src/main/java/io/axoniq/axonserver/connector/ResultStreamPublisher.java
Outdated
Show resolved
Hide resolved
src/main/java/io/axoniq/axonserver/connector/ResultStreamPublisher.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A bunch of minor pointers to chat about.
And, don't forget the code smell SonarCloud marked.
It's about the generic name you've used, so I would anticipate it to be resolved after processing the comments anyhow.
src/main/java/io/axoniq/axonserver/connector/ResultStreamPublisher.java
Outdated
Show resolved
Hide resolved
src/main/java/io/axoniq/axonserver/connector/ResultStreamPublisher.java
Outdated
Show resolved
Hide resolved
src/main/java/io/axoniq/axonserver/connector/ResultStreamPublisher.java
Outdated
Show resolved
Hide resolved
src/main/java/io/axoniq/axonserver/connector/ResultStreamPublisher.java
Outdated
Show resolved
Hide resolved
src/test/java/io/axoniq/axonserver/connector/ResultStreamPublisherTest.java
Show resolved
Hide resolved
src/test/java/io/axoniq/axonserver/connector/admin/AdminChannelIntegrationTest.java
Show resolved
Hide resolved
…given when this method is executed).
…ule that has been tested.
Register the onAvailable callback outside the constructor.
Kudos, SonarCloud Quality Gate passed! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My concerns have been addressed, hence approving.
No description provided.