-
Notifications
You must be signed in to change notification settings - Fork 198
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
Split roundstate saving in two #2046
Conversation
Coverage from tests in coverage: 51.0% of statements across all listed packagescoverage: 63.2% of statements in consensus/istanbul coverage: 42.7% of statements in consensus/istanbul/announce coverage: 56.0% of statements in consensus/istanbul/backend coverage: 0.0% of statements in consensus/istanbul/backend/backendtest coverage: 24.3% of statements in consensus/istanbul/backend/internal/replica coverage: 66.0% of statements in consensus/istanbul/core coverage: 50.0% of statements in consensus/istanbul/db coverage: 0.0% of statements in consensus/istanbul/proxy coverage: 64.4% of statements in consensus/istanbul/uptime coverage: 51.8% of statements in consensus/istanbul/validator coverage: 79.2% of statements in consensus/istanbul/validator/random |
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.
This PR is missing transition logic from old db format to new. I mean if we run this on a old client it will fail.
Still for testing purposes it's ok
@@ -40,6 +41,7 @@ const ( | |||
dbVersionKey = "version" // Version of the database to flush if changes | |||
lastViewKey = "lastView" // Last View that we know of | |||
rsKey = "rs" // Database Key Pefix for RoundState | |||
rcvdKey = "rcvd" // Database Key Prefix for rcvd messages from the RoundState (split saving) |
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.
why not have differente keys for prepares, commits and parent commits?
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 believe they shouldn't have an outstanding size, though I agree that further splitting is better. I'll check how the code looks with it.
0dafa70
to
025eb88
Compare
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #2046 +/- ##
==========================================
+ Coverage 54.20% 54.22% +0.01%
==========================================
Files 692 692
Lines 115463 115618 +155
==========================================
+ Hits 62585 62691 +106
- Misses 49050 49082 +32
- Partials 3828 3845 +17
... and 25 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
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.
Other than the log, and the nit in the test, LGTM
defer ms.messagesMu.Unlock() | ||
var added = 0 | ||
for _, msg := range msgs { | ||
err := ms.checkValidToAdd(msg) |
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.
Should we log the invalid messages or the possible overrides?
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 don't think so, since it's actually expected that there will be some overrides on a load
view := newView(2, 1) | ||
rs := newRoundState(view, valSet, valSet.GetByIndex(0)) | ||
rs.AddPrepare(mockViewMsg(view, istanbul.MsgPrepare, valSet.GetByIndex(0).Address())) | ||
rs.AddCommit(mockViewMsg(view, istanbul.MsgPrepare, valSet.GetByIndex(0).Address())) |
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.
Shouldn't this be a istanbul.MsgCommit? (same with the next one)
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.
yes, missed that
5b297b3
to
bb16fb8
Compare
* Split roundstate saving in two: prepares/commit/parentCommit messages on one side.
* Split roundstate saving in two: prepares/commit/parentCommit messages on one side.
Split the roundstate saving, allowing consensus message processing to be a bit faster.