-
Notifications
You must be signed in to change notification settings - Fork 474
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
Catchpoints: Enrich catchpoint generation and status with KV metadata #4808
Conversation
Test that old data is populated by roundCowState.deltas()
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 this is the right approach. I suspect it would be possible to get the number of kvs for the catchpoint from the iterator rather that the select count
, but if it takes any effort to restructure, this seems fine.
Codecov Report
@@ Coverage Diff @@
## master #4808 +/- ##
==========================================
+ Coverage 54.65% 54.67% +0.01%
==========================================
Files 416 417 +1
Lines 53710 53734 +24
==========================================
+ Hits 29357 29379 +22
+ Misses 21923 21922 -1
- Partials 2430 2433 +3
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
As of 7e1f45c, here's a proof point against betanet. Generating catchpoints with Total KVs in file header:
Applying a catchpoint generated by a node running the PR:
|
As of 85e25e9, applying a catchpoint generated by a node running the PR yields the processed == verified counts:
|
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 feel strongly about swapping the order of kv, accounts all over the place, so leaving an approval.
Earlier we talked about not exporting some stuff, and the KV had to remain exported for some reason. With that in mind, I'd probably add something to those names, as that means Account
is now an exported symbol. Seems pretty weird to me. Maybe AccountHashKind, etc?
I hate going overboard on long names - but these are weird little things that don't feel like they deserve such important names.
could you make a minimal diff by re-merging master after |
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.
The change looks good, some code-style comments
Enriches catchpoints with the following KV metadata mirroring accounts:
CatchpointFileHeader
CatchpointGenerationEvent
/v2/status
goal node status
Prior to the PR, account and KV hashes are not disambiguated. Consequently, it's possible to see catchpoint accounts verified > total accounts like the example below: