-
Notifications
You must be signed in to change notification settings - Fork 16
fix: Update PublishHandler to close only the stream on BEHIND #1915
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Nana Essilfie-Conduah <[email protected]>
Signed-off-by: Nana Essilfie-Conduah <[email protected]>
ata-nas
left a comment
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.
Just some preliminary notes to think about.
| } else { | ||
| // close stream leaving connection open for subsequent publishers calls | ||
| if (handleResult.shouldCloseStream()) { | ||
| replies.onComplete(); | ||
| } | ||
|
|
||
| // clean up handler state | ||
| if (handleResult.shouldReset()) { | ||
| resetState(); | ||
| } |
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.
Not sure if these should be in an else. All these booleans should probably run if they are all true.
| } else { | ||
| // close stream leaving connection open for subsequent publishers calls | ||
| if (handleResult.shouldCloseStream()) { | ||
| replies.onComplete(); |
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.
Not sure if that is the only thing we need to do to close the stream. I remember there were some changes recently that allow us to actually close the stream completely.
| assertThat(tooFarBehindResponseObserver.getOnErrorCalls()).isEmpty(); | ||
| assertThat(tooFarBehindResponseObserver.getOnSubscriptionCalls()).isEmpty(); | ||
| assertThat(tooFarBehindResponseObserver.getOnCompleteCalls().get()).isEqualTo(1); | ||
| assertThat(tooFarBehindResponseObserver.getOnErrorCalls().size()).isEqualTo(0); |
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.
Just FYI, assertj has some conveniences for zero values, some other very frequently used. So we can do "assertThat(number).isZero();". Non-blocking nit. On another note, instead of calling Collection#size and checking if it is zero, we could simply call assertThat(collection).isEmpty(); as seen in L:618. Again, totally non-blocking nit.
Codecov Report❌ Patch coverage is
@@ Coverage Diff @@
## main #1915 +/- ##
============================================
- Coverage 79.48% 79.09% -0.39%
- Complexity 1193 1200 +7
============================================
Files 128 130 +2
Lines 5684 5770 +86
Branches 610 616 +6
============================================
+ Hits 4518 4564 +46
- Misses 887 924 +37
- Partials 279 282 +3
... and 2 files with indirect coverage changes 🚀 New features to boost your workflow:
|
Reviewer Notes
Currently the handler closes both the stream and connection when handling BEHIND code.
However, the expectation is the publisher would follow up either with more appropriate block items or a TOO_FAR_BEHIND.
When the publisher does it experiences a socket closed exception
handleBlockActionResult()to manage 3 cases in a more coordinate fashion and not call either unnecessarilyPartially address #1914 but does not attempt to fix the protocol definition as that requires CN coordination and larger changes
Related Issue(s)
Use keywords
Fix, Fixes, Fixed, Close, Closes, Closed, Resolve, Resolves, Resolvedto connect an issue to be closed by this pull request.