storage: misc fixes after excise cluster setting addition #111678
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
cluster-setting-excise-fixes
branch
from
695e58b to
5dbc080
Compare
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @itsbilal and @jbowens)
pkg/kv/kvserver/store_snapshot.go line 507 at r1 (raw file):
keyRanges := rditer.MakeReplicatedKeySpans(header.State.Desc) // NB: V23_2_StmtDiagForPlanGist is one after V23_2_PebbleFormatVirtualSSTables
This is ugly.. If I understand correctly, we should have reserved 2 versions?
I would add a const V23_2_PebbleFormatVirtualSSTablesDone (or whatever) in clusterversion and make it equal to V23_2_PebbleFormatVirtualSSTables + 1 and explain in the comment why it's defined like this (we didn't want to shift the keys). Though I don't think shifting the 23_2 keys should be a problem. Or if we can define it at the end, that'd be better too.
There was a problem hiding this comment.
Reviewable status:
pkg/kv/kvserver/store_snapshot.go line 507 at r1 (raw file):
Previously, RaduBerinde wrote…
This is ugly.. If I understand correctly, we should have reserved 2 versions?
I would add a const
V23_2_PebbleFormatVirtualSSTablesDone(or whatever) inclusterversionand make it equal toV23_2_PebbleFormatVirtualSSTables + 1and explain in the comment why it's defined like this (we didn't want to shift the keys). Though I don't think shifting the 23_2 keys should be a problem. Or if we can define it at the end, that'd be better too.
I could define it at the end but I prefer the alias approach a little more, so that's what I did. @jbowens what do you think?
itsbilal
force-pushed
the
cluster-setting-excise-fixes
branch
2 times, most recently
from
7e139fb to
bb2b7b2
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/kv/kvserver/store_snapshot.go line 507 at r1 (raw file):
Previously, itsbilal (Bilal Akhtar) wrote…
I could define it at the end but I prefer the alias approach a little more, so that's what I did. @jbowens what do you think?
I do think a separate cluster version is preferable so that each new cluster version is only enabling 1 new piece of functionality.
itsbilal
force-pushed
the
cluster-setting-excise-fixes
branch
from
bb2b7b2 to
0603319
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/kv/kvserver/store_snapshot.go line 507 at r1 (raw file):
Previously, jbowens (Jackson Owens) wrote…
I do think a separate cluster version is preferable so that each new cluster version is only enabling 1 new piece of functionality.
Done.
This change addresses some follow-up code review points from cockroachdb#111437. One of these was to update a cluster setting's description to better reflect its interaction with another cluster setting, and another is to address one missing guard in cluster versions when ratcheting forward. Epic: none Release note: None
itsbilal
force-pushed
the
cluster-setting-excise-fixes
branch
from
0603319 to
09a57e6
Compare
There was a problem hiding this comment.
Reviewed 1 of 3 files at r1, 2 of 3 files at r2, 3 of 3 files at r4, all commit messages.
Reviewable status:
|
TFTR! bors r=jbowens |
|
Build failed (retrying...): |
|
Build failed (retrying...): |
|
Build failed (retrying...): |
|
Build succeeded: |
This change addresses some follow-up code review points from #111437. One of these was to update a cluster setting's description to better reflect its interaction with another cluster setting, and another is to address one missing guard in cluster versions when ratcheting forward.
Epic: none
Release note: None