storage: Write sstables in v4 format for shared ingestions #108866
Conversation
|
It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR? 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
|
This change is |
itsbilal
force-pushed
the
sst-format-disagg
branch
from
53e2141 to
33e023d
Compare
We require sstables to be written with obsolete bits for shared sstable ingestion to work correctly. This change moves to writing sstables with the new format if the cluster version is high enough. Unblocks cockroachdb#107394. Epic: none Release note: None
itsbilal
force-pushed
the
sst-format-disagg
branch
from
33e023d to
ee5cce5
Compare
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @itsbilal)
pkg/storage/sst_writer.go line 77 at r1 (raw file):
format = sstable.TableFormatPebblev3 } if cs.Version.IsActive(ctx, clusterversion.V23_2_PebbleFormatVirtualSSTables) && ValueBlocksEnabled.Get(&cs.SV) {
Seems like things will be fragile w.r.t to the value-blocks-enabled setting? Like if it's temporarily turned off, we will hit an error at restore time?
|
Previously, RaduBerinde wrote…
Value blocks default to on and are really only a cluster setting for metamorphic testing it seems. I didn't want to eliminate that surface area of tests by forcing the use of v4 for all new builds, so I kept it this way. We can revisit this closer to when disagg storage moves out of tech preview (by when value blocks would have gotten much more testing). The error will really only be hit when we ingest a shared table that's not format v4. |
There was a problem hiding this comment.
Reviewable status:
pkg/storage/sst_writer.go line 77 at r1 (raw file):
Previously, itsbilal (Bilal Akhtar) wrote…
Value blocks default to on and are really only a cluster setting for metamorphic testing it seems. I didn't want to eliminate that surface area of tests by forcing the use of v4 for all new builds, so I kept it this way. We can revisit this closer to when disagg storage moves out of tech preview (by when value blocks would have gotten much more testing). The error will really only be hit when we ingest a shared table that's not format v4.
Makes sense, thanks!
|
TFTR! bors r=RaduBerinde |
|
Build failed (retrying...): |
|
Build succeeded: |
We require sstables to be written with obsolete bits for shared sstable ingestion to work correctly. This change moves to writing sstables with the new format if the cluster version is high enough.
Unblocks #107394.
Epic: none
Release note: None