-
Notifications
You must be signed in to change notification settings - Fork 179
feat: Close block node connections at block boundaries #21903
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
feat: Close block node connections at block boundaries #21903
Conversation
Signed-off-by: Tim Farber-Newman <[email protected]>
Signed-off-by: Tim Farber-Newman <[email protected]>
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
…dary Signed-off-by: Tim Farber-Newman <[email protected]>
Signed-off-by: Tim Farber-Newman <[email protected]>
Signed-off-by: Tim Farber-Newman <[email protected]>
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferences |
Codecov Report❌ Patch coverage is
@@ Coverage Diff @@
## main #21903 +/- ##
============================================
+ Coverage 70.82% 70.84% +0.01%
Complexity 24445 24445
============================================
Files 2675 2675
Lines 104415 104444 +29
Branches 10960 10964 +4
============================================
+ Hits 73954 73989 +35
+ Misses 26424 26421 -3
+ Partials 4037 4034 -3
... and 2 files with indirect coverage changes 🚀 New features to boost your workflow:
|
joshmarinacci
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.
I approve but I'd also like to have someone who's more familiar with Block Node Communication review it as well.
Signed-off-by: Tim Farber-Newman <[email protected]>
Signed-off-by: Tim Farber-Newman <[email protected]>
Signed-off-by: Tim Farber-Newman <[email protected]>
Signed-off-by: Tim Farber-Newman <[email protected]>
…dary Signed-off-by: Tim Farber-Newman <[email protected]>
Signed-off-by: Tim Farber-Newman <[email protected]>
Signed-off-by: Tim Farber-Newman <[email protected]>
…dary Signed-off-by: Tim Farber-Newman <[email protected]>
…dary Signed-off-by: Tim Farber-Newman <[email protected]>
…dary Signed-off-by: Tim Farber-Newman <[email protected]>
Signed-off-by: Tim Farber-Newman <[email protected]>
Signed-off-by: Tim Farber-Newman <[email protected]>
…dary Signed-off-by: Tim Farber-Newman <[email protected]>
Signed-off-by: Tim Farber-Newman <[email protected]>
.../hedera-app/src/main/java/com/hedera/node/app/blocks/impl/streaming/BlockNodeConnection.java
Show resolved
Hide resolved
.../hedera-app/src/main/java/com/hedera/node/app/blocks/impl/streaming/BlockNodeConnection.java
Outdated
Show resolved
Hide resolved
petreze
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.
A more general question. As far as I remember, we never really want to have more than one BN that we are streaming to (if nothing has changed). Would it be possible to remove this overlap of the new active connection and the one that is finishing up streaming the current block?
We could be waiting for that current block to be streamed, close the connection meant to be closed, and choose a new active connection so that we do not stream simultaneously to two different nodes
.../hedera-app/src/main/java/com/hedera/node/app/blocks/impl/streaming/BlockNodeConnection.java
Show resolved
Hide resolved
I don't think there is anything wrong with having two connections open at the same time: one active and one pending close. Immediately following the block being sent, the old connection will close itself. The problem with waiting for the other block to finish streaming before opening the new connection is if the old connection takes a long time, it will negatively impact the CN (saturation will increase). In performance testing we've seen periods where it takes upwards of five seconds to send a single request to the block node. If there are multiple requests that we still need to send and they all take that long, we would have produced several more blocks leading us to be even further behind. This also made the connection scheduling more complicated and brittle since you would need to track the pending new connection and deal with potentially rescheduling connection tasks multiple times to finally do the switch. |
Signed-off-by: Tim Farber-Newman <[email protected]>
…dary Signed-off-by: Tim Farber-Newman <[email protected]>
Signed-off-by: Tim Farber-Newman <[email protected]>
Description:
Instead of ungracefully closing block node connections, this PR attempts to allow connections to gracefully close by doing two things:
EndStream.Code.RESETcode to notify the block node that we are closing the connection.Note: Technically with these changes, multiple connections may be streaming to block nodes at the same time: the "active" connection as observed by the connection manager, and the connection waiting to be closed after the current block is finished being sent. The overlap should be fairly minimal and just for as long as it takes to finish streaming the block.
Related issue(s):
Fixes #21878
Notes for reviewer:
Checklist