-
Notifications
You must be signed in to change notification settings - Fork 311
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
Enable IndexV2 #4583
Enable IndexV2 #4583
Conversation
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.
In a follow up PR, I'll introduce a test case that exercises the fallback deserialization logic, where data in the blockstore is written as IndexFallback, deserialized as such, and recovered as Index.
I'm inclined to say that we should include the test in this PR. Since this PR is now writing the new format (and should land in v2.2), the test you described sounds similar to what we'd encounter in the v2.1 to v2.2 upgrade scenario if I'm not mistaken:
- Node is running with v2.1 and writing BTreeSet
- Node is stopped and upgraded to v2.2
- Node is restarted with v2.2 and reads the BTreeSet index type for a slot that was written when it was running v2.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.
I would typically recommend staging multiple small PRs to:
- change the name from
Index
-->IndexV1
and setup thepub type Index = IndexV1;
- Switch the mapping so
pub type Index = IndexV2;
However, this is small enough I'm not going to gate on it.
Regarding naming, IndexLegacy
makes it more clear that this type is being phased out, which is good. But then compared to ShredIndexV2
it feels like we're mixing 1/b/iii. Especially if we ever add a ShredIndexV3
.
I also agree with @steviez that we should make sure we have the unit tests for the conversions/fallbacks in place as part of this PR (or before it)
0ce665a
to
199ec83
Compare
Good call, this is definitely a better naming scheme. |
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
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.
Code looks good. One additional thing that I think would be nice to add is an entry to the CHANGELOG. Namely, something similar to what is being added in #4600 like this:
- Blockstore Index column format change
- The Blockstore Index column format has been updated. A Blockstore written to with v2.2 is compatible with v2.1 and incompatible with v2.0 and older
I think under the "Breaking" section (which that PR introduces) is the right place. That other PR should land very shortly, so the hope is maybe you could rebase on top of it + add the extra line to the changelog then
e462086
to
b9eb826
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.
LGTM
Problem
The current blockstore index type, backed by a
BTreeSet
, suffers from performance issues in serialization / deserialization due to its dynamically allocated and balanced nature. See #3570 and #3900 for context.Summary of Changes
A follow up of #3900, this enables
IndexV2
as the primary read/write target for the blockstore'sIndex
type.Other notes
In a follow up PR, I'll introduce a test case that exercises the fallback deserialization logic, where data in the blockstore is written as
IndexFallback
, deserialized as such, and recovered asIndex
.