[dnm] use clusterversion.Refer to tie arbitrary code to cluster versions#90428
[dnm] use clusterversion.Refer to tie arbitrary code to cluster versions#90428tbg wants to merge 1 commit intocockroachdb:masterfrom
Conversation
Release note: None
|
This change is |
| // TODO(irfansharif): Remove this field in the release after the one where we | ||
| // stop consulting the protobuf copy of the replica state when applying a | ||
| // snapshot. | ||
| // This field can be removed together with |
There was a problem hiding this comment.
The comment here is interesting, btw. We thought we had figured out when this was safe to remove, but it wasn't actually true yet. No amount of version gating would've fixed that.
| import "github.com/cockroachdb/cockroach/pkg/clusterversion" | ||
|
|
||
| func init() { | ||
| // In the real world, when the field below was deprecated, we'd have |
There was a problem hiding this comment.
2¢: Eh, I donno if I agree with this comment's assertion that we should be linking to a named version?
Let's say that ReplicaState{}.DeprecatedUsingAppliedStateKey is still referenced by/used by 22.1, so we need to keep it as long as 22.1 is in the cluster, but can get rid of it once we're on 22.2+ binaries (i.e. the cluster verison is >22.1).
In that case, I'd Refer to to clusterversion.V22_1, since that's the maximum version where we still need the field, and when we delete that version, we can delete the field.
|
@tbg You inclined to de-DNM this and merge it? I think it's a good pattern to push. My only suggestion would be to extend the comment on Refer with an example, and adjust example usage, in which Refer points to the minimum final version where the field is still used rather than a named intermediate upgrade version, i.e. |
celiala
left a comment
There was a problem hiding this comment.
I couldn't tell if we really wanted to tie ReplicaState{}.DeprecatedUsingAppliedStateKey to clusterversion.Start22_2, or if this was purely an example to show how .Refer() might work. So I pulled this .Refer() example, along with @dt 's suggestions of adding examples into a separate PR.
Would appreciate a look to ensure I've captured everyone's suggestions:
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @dt and @tbg)
Release note: None