-
Notifications
You must be signed in to change notification settings - Fork 179
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
[KVStore] Integration test for version upgrade #5477
[KVStore] Integration test for version upgrade #5477
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feature/protocol-state-kvstore #5477 +/- ##
===================================================================
- Coverage 56.02% 44.38% -11.64%
===================================================================
Files 1025 114 -911
Lines 98784 13515 -85269
===================================================================
- Hits 55339 5998 -49341
+ Misses 39303 7068 -32235
+ Partials 4142 449 -3693
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
to f59efe400a8bd77c29a6124f797b466ae4927c15
…an/5326-integration
- require consecutive versions - bootstrap with v0 rather than v1 - ignore invalid service events
- don't panic on unknown service events when determining metrics - added todo to clean up metrics
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.
very impressive test -- I liked how compact/highly-modular the testing code is for verifying 2 upgraders from initial to supported to unsupported version 👏
@@ -877,6 +877,7 @@ func (m *FollowerState) epochTransitionMetricsAndEventsOnBlockFinalized(block *f | |||
// Suppose an EpochSetup service event is emitted during execution of block A. C seals A, therefore | |||
// we apply the metrics/events when C is finalized. The first block of the EpochSetup | |||
// phase is block C. | |||
// TODO: this should read data from the KVStore/ProtocolStateEntry for the block rather than parsing service events |
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.
lets capture this in an issue. I am worried that if we forget that, there might be an unnecessary pager duty alert going out
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.
👍 It's captured in #5666.
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.
Very nice, I like how compact and awaiting style your test is.
d1faa23
into
feature/protocol-state-kvstore
This PR adds an integration test for the protocol state version upgrade process.
InvalidEpochTransitionAttempted
field from KVStoreRemaining TODOs
InvalidEpochTransitionAttempted
field from kv store (duplicated from epoch protocol state entry)