storage: require min version file and deprecate cluster version key #97054
Conversation
|
This change is |
RaduBerinde
force-pushed
the
no-version-key
branch
3 times, most recently
from
9c6611e to
9091480
Compare
RaduBerinde
changed the title
RaduBerinde
requested review from
smg260,
renatolabs,
sumeerbhola,
jbowens and
tbg
and removed request for
a team
RaduBerinde
force-pushed
the
no-version-key
branch
from
9091480 to
9335d07
Compare
pkg/storage/pebble.go
Outdated
| @@ -1078,32 +1078,43 @@ func NewPebble(ctx context.Context, cfg PebbleConfig) (p *Pebble, err error) { | |||
|
|
|||
| db, err := pebble.Open(cfg.StorageConfig.Dir, opts) | |||
| if err != nil { | |||
| if !minVerFileExists && strings.Contains(err.Error(), "already exists") { | |||
There was a problem hiding this comment.
strings.Contains? Wouldn't we want a structured error here? An argument could be made that it isn't worth it because it's such a dead code path & the event of a false positive isn't dramatic since the startup was failing either way?
There was a problem hiding this comment.
Yeah I would add a better way in Pebble. But for now I'm not sure of the higher level logic - what are the expectations if a machine fails during bootstrap? If it fails after creating the store but before creating the min version file, we would need manual cleanup before trying to bootstrap the store again. OTOH if we allow an existing store, then we might open an old version. Maybe the right thing is to add an ErrorIfNotEmpty mode to Pebble?
RaduBerinde
force-pushed
the
no-version-key
branch
2 times, most recently
from
89e2b5a to
142cca9
Compare
RaduBerinde
force-pushed
the
no-version-key
branch
2 times, most recently
from
f455c8c to
5b3ebf0
Compare
There was a problem hiding this comment.
Nice
Reviewed 3 of 5 files at r1, 5 of 5 files at r2, 16 of 16 files at r3, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @renatolabs, @smg260, @sumeerbhola, and @tbg)
|
TFTR! |
This change makes the min version file a requirement for opening an existing store. We have been writing this file out for at least two releases, so this doesn't affect our compatibility story. Release note: None Epic: none
This change deprecates the cluster version key. We have the version outside the store (in the min version file) so we no longer need to store it inside a KV. To keep backward compatibility with 22.2, we still write the key but we never read it. Once we stop supporting 22.2, we can stop writing it altogether. This required updating the `TestClusterVersionWriteSynthesize` to use versions that work with the binary. We also clean up the test and move it to `kvstorage` where it belongs. Release note: None Epic: none
RaduBerinde
force-pushed
the
no-version-key
branch
from
5b3ebf0 to
d1a1924
Compare
|
bors r+ |
|
Build succeeded: |
storage: require min version file
This change makes the min version file a requirement for opening an
existing store.
We have been writing this file out for at least two releases, so this
doesn't affect our compatibility story.
Release note: None
Epic: none
storage: deprecate cluster version key
This change deprecates the cluster version key. We have the version
outside the store (in the min version file) so we no longer need to
store it inside a KV.
To keep backward compatibility with 22.2, we still write the key but
we never read it. Once we stop supporting 22.2, we can stop writing it
altogether.
This required updating the
TestClusterVersionWriteSynthesizeto useversions that work with the binary. We also clean up the test and move
it to
kvstoragewhere it belongs.Release note: None
Epic: none
Fixes #42653.