db: Add FormatMajorVersion for virtual sstables #2600
Conversation
|
This change is |
There was a problem hiding this comment.
We only check the version against FormatVirtualSSTables in one place, and it's an after-the-fact assertion. Shouldn't we check for it in the code path that creates virtual SSTs?
Reviewable status: 0 of 8 files reviewed, 2 unresolved discussions (waiting on @itsbilal and @jbowens)
format_major_version.go line 164 at r1 (raw file):
// FormatVirtualSSTables is a format major version that adds support for // virtual sstables that can reference a sub-range of keys in an underlying // physical sstables. This information is persisted through new,
[nit] sstable
format_major_version.go line 166 at r1 (raw file):
// physical sstables. This information is persisted through new, // backward-incompatible fields in the Manifest, and therefore requires // an FMV.
[nit] "FMV" is not used anywhere in these comments, I'd expand to "format major version"
There was a problem hiding this comment.
Reviewed 8 of 8 files at r1, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @itsbilal)
format_major_version.go line 167 at r1 (raw file):
// backward-incompatible fields in the Manifest, and therefore requires // an FMV. FormatVirtualSSTables
nit: can we prefix with "Experimental" too for now?
This change adds a specific FormatMajorVersion specifically for the virtual sstable work, as the changes in cockroachdb#2484 are not backward compatible. There's a guard in version application that will error out if we create a backing table without having an appropriate FMV. Fixes cockroachdb#2557.
itsbilal
force-pushed
the
fmv-virtual-ssts
branch
from
e26643b to
cd55934
Compare
There was a problem hiding this comment.
TFTR!
@RaduBerinde I agree that we should gate the virtual sstable write paths with this. However none of the write paths are currently merged; once this PR merges, I'll update both #2538 and #2582 to gate their writes on this.
Reviewable status: 4 of 8 files reviewed, all discussions resolved (waiting on @jbowens)
format_major_version.go line 164 at r1 (raw file):
Previously, RaduBerinde wrote…
[nit] sstable
Done.
format_major_version.go line 166 at r1 (raw file):
Previously, RaduBerinde wrote…
[nit] "FMV" is not used anywhere in these comments, I'd expand to "format major version"
Done.
format_major_version.go line 167 at r1 (raw file):
Previously, jbowens (Jackson Owens) wrote…
nit: can we prefix with "Experimental" too for now?
Done.
This change adds a specific FormatMajorVersion specifically for the virtual sstable work, as the changes in #2484 are not backward compatible. There's a guard in version application that will error out if we create a backing table without having an appropriate FMV.
Fixes #2557.