-
Notifications
You must be signed in to change notification settings - Fork 180
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
[Storage Refactor] Refactor ConsumerProgress #6872
base: master
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6872 +/- ##
==========================================
+ Coverage 41.11% 41.15% +0.03%
==========================================
Files 2116 2118 +2
Lines 185749 185815 +66
==========================================
+ Hits 76378 76474 +96
+ Misses 102954 102920 -34
- Partials 6417 6421 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
86bca71
to
fa24125
Compare
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.
May I suggest a minor documentation upgrade of line 47 to further improve accuracy of documentation:
// - codes.ErrIncorrectValue - if stored value is larger than processed. |
Let current
denote the counter's current value and processed
is the new value.
- According to the documentation, we error
codes.ErrIncorrectValue
ifcurrent
>processed
. This implies that aSet(processed)
operation whereprocessed = current
should succeed. - The error describes, in which cases we reject a new element. Of course, we reject the new element (
processed
), when it is smaller - this is correctly reflected in the documentation. But we also reject elements that have the same value, which is missing from the documentation. - Note that this is the difference between monotonous increasing and strict monotonous increasing. Monotonous allows values to repeat (they just can't get smaller). E.g. 8, 12, 13, 13, 13, 100 is a monotonously increasing sequence. But it is not strictly monotonous increasing, because some elements have the same value (13) as the prior element, which is not allowed for strict monotonous increasing.
I would suggest to replace line 47 by
// - codes.ErrIncorrectValue - if stored value is ≥ processed (requirement of strict monotonous increase is violated).
return err | ||
} | ||
|
||
lastFullBlockHeight, err = counters.NewPersistentStrictMonotonicCounter(progress) |
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.
NewPersistentStrictMonotonicCounter
now only takes a consumer progress that has been initialized. The initialization steps is moved out of the PersistentStrictMonotonicCounter
, so that the initialization logic can be reused by other modules, and forces the caller to initialize the progress before being used by the counter.
It also simplifies the counter, as it doesn't need to worry about whether the progress has been initialized or not, since it must have been initialized.
@@ -0,0 +1,87 @@ | |||
package store |
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 is actually renamed from storage/badger/consumer_progress.go
, github wasn't able to recognize this move, but think it's a new file.
@@ -0,0 +1,58 @@ | |||
package store |
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 is actually renamed from storage/badger/consumer_progress_test.go, github wasn't able to recognize this move, but think it's a new file.
This PR refactors consumer progress to use badger batch updates instead of transactions.
Also created a ConsumerProgressInitializer to separate the initialization process.