-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Broadcast gossip announcements in sub batches #2985
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
Conversation
cfromknecht
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.
@johng thanks for the PR, great work!! i've completed an initial first pass, and the functionality seems to be correct. apart from one or two reliability comments, the rest are mostly around simplifications and adhering to the style guidelines :)
cfromknecht
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.
@johng thanks for the quick turn around. completed another pass, mostly nits and bringing the PR in line with the style guidelines. appreciate the thorough testing of the batch splits 👍
|
Gonna start running this patch on my node and see how it fairs |
|
@johng could you squash the fixup commits and rebase? |
6ec82ec to
a9d3db7
Compare
wpaulino
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.
Mostly just nits from me on commit structure.
discovery/gossiper.go
Outdated
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.
Nit: s/batches/batch/
discovery/gossiper.go
Outdated
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.
Nit: renaming these to annBatch and splitAnnBatch can help gain some more line space.
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 think I thought the more read able variable name is more important here
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 think ann is a reasonable abbreviation for announcement but fine with me either way.
discovery/gossiper_test.go
Outdated
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.
Nit: looks like the TestSplitAnnouncementCorrectSubBatches changes are a fixup of f4a9edc?
discovery/gossiper_test.go
Outdated
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.
Nit: this fixup should be applied to f4a9edca7e107e9af31ad8b44c8ed9c5f78c2792 since that's where the changes were originally introduced.
|
@wpaulino following your comments, and since this was a small change for the gossiper I've combined the two testing commits into one so that all commits compile. |
cfromknecht
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.
@johng been testing this for a bit on my node, everything appears to be working smoothly! one final comment from me, see inline
discovery/gossiper.go
Outdated
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 think it'd be preferable to do this computation entirely using integer arithmetic, instead of using floating points and math lib functions. current calculation does ceil(batchSize * subBatchDelay / totalDelay), which is equivalent to using ceiling integer division:
subBatchSize := (batchSize*subBatchDelay + totalDelay - 1) / totalDelaytypically we try to avoid using floats when possible, as behavior the is less consistent across architectures
|
thanks @johng latest changes lgtm! ready to be squashed |
cfromknecht
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.
Awesome work @johng! Really happy with how this turned out. LGTM 🍻
Will need one more rebase (there were some issues with our build that have now been fixed), if you pull in master it should get a clean run on travis and be merge-ready. Thanks!
wpaulino
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.
LGTM 🎱
A few non-blocking nits left inline -- would prefer if the commits were squashed into one as we generally favor each commit being able to build on its own.
discovery/gossiper.go
Outdated
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 think ann is a reasonable abbreviation for announcement but fine with me either way.
| return batchSize | ||
| } | ||
|
|
||
| subBatchSize := (int(batchSize)*int(subBatchDelay) + int(totalDelay) - 1) / |
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.
Nit: unnecessary cast on batchSize.
In this PR we split a list of announcements in the gossiper into sub batches. These are broadcasted at regular intervals instead of all at once.
See #2979
Pull Request Checklist
Contribution Guidelines
the positive and negative (error paths) conditions (if applicable)
the bug being fixed to prevent regressions
logging level
go fmt(the tab character should be counted as 8 characters, not 4, as some IDEs do
per default)
make checkdoes not fail any testsgo vetdoes not report any issuesmake lintdoes not report any new issues that did notalready exist
cases it can be justifiable to violate this condition. In that case, the
reason should be stated in the commit message.