-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Improve getBlocks request handling #4851
Improve getBlocks request handling #4851
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.
utACK
core/src/main/java/bisq/core/dao/node/lite/network/RequestBlocksHandler.java
Outdated
Show resolved
Hide resolved
@@ -168,31 +167,36 @@ public void onFailure(@NotNull Throwable throwable) { | |||
@Override | |||
public void onMessage(NetworkEnvelope networkEnvelope, Connection connection) { | |||
if (networkEnvelope instanceof GetBlocksResponse) { |
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.
if (networkEnvelope instanceof GetBlocksResponse) { | |
if (!(networkEnvelope instanceof GetBlocksResponse)) return; // Also need to remove other brace |
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 prefer to not return early here as this style is commonly used in many places and often there are several if else branches. It is more like a dispatcher or switch case style (if switch would be more flexible in java). I prefer to use the early return for validation use cases but here its more to select the msg we are interested in.
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.
That's a good point. Might be cleaner to make a handleGetBlocksResponse(GetBlocksResponse r)
and other respective dispatch handlers like is done in the trade protocol for these cases.
core/src/main/java/bisq/core/dao/node/lite/network/RequestBlocksHandler.java
Outdated
Show resolved
Hide resolved
@sqrrm Is there anything else you want @chimp1984 to address? |
@ripcurlx I have nothing more but in private @chimp1984 mentioned he might want to add some more so I'll wait for that to merge. |
@ripcurlx I would like to do myself another review before merging it. |
cb9ddb1
to
357c980
Compare
Rename params Cleanups
Add checks to tryWithNewSeedNode method Cleanups
Cleanups
Cleanups
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.
utACK
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.
utACK
No description provided.