keys: remove compatibility with pre-VersionMeta2Splits nodes #26967
Conversation
|
This change brings up an interesting question. We don't have a defined process for what to do when old |
|
This change is |
nvanbenschoten
force-pushed
the
nvanbenschoten/meta2splits
branch
from
3dbbd27
to
d6d0a36
Compare
|
I think it's good idea to go back and remove old VersionKeys (and their associated runtime checks) when they're no longer needed. But we should probably go in order - don't remove VersionMeta2Splits before we've removed all the old ones. Reviewed 12 of 12 files at r1. Comments from Reviewable |
complete! 0 of 0 LGTMs obtained (and 1 stale)
nvanbenschoten
force-pushed
the
nvanbenschoten/meta2splits
branch
from
d6d0a36
to
2dbbe01
Compare
This change removes the compatibility layer introduced with `VersionMeta2Splits` that conditionally decided whether splitting meta2 ranges was permitted. In doing so, it cleans up some plumbing code that is no longer needed in 2.1 binaries. Release note: None
nvanbenschoten
force-pushed
the
nvanbenschoten/meta2splits
branch
from
2dbbe01
to
e1bfabd
Compare
That sounds reasonable to me. I went through and marked unused version keys (those that have had their associated runtime checks removed) as such. I also added instructions on deleting unstable versions. PTAL @ the new commit. |
|
Review status: pkg/settings/cluster/cockroach_versions.go, line 37 at r3 (raw file):
Why shouldn't major or minor versions be removed? I realized this is missing a step: you need to adjust the starting point of the Comments from Reviewable |
This provides a set of instructions on deleting a `VersionKey`. They mirror the existing instructions on adding a `VersionKey`. Release note: None
nvanbenschoten
force-pushed
the
nvanbenschoten/meta2splits
branch
from
e1bfabd
to
822965b
Compare
|
Review status: pkg/settings/cluster/cockroach_versions.go, line 37 at r3 (raw file):
Removed that.
I considered this but convinced myself that it's not actually an issue. Comments from Reviewable |
|
Review status: pkg/settings/cluster/cockroach_versions.go, line 37 at r3 (raw file): Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
Ack. I forgot that the persisted values are elsewhere. Comments from Reviewable |
|
bors r+ |
26967: keys: remove compatibility with pre-VersionMeta2Splits nodes r=nvanbenschoten a=nvanbenschoten This change removes the compatibility layer introduced with `VersionMeta2Splits` that conditionally decided whether splitting meta2 ranges was permitted. In doing so, it cleans up some plumbing code that is no longer needed in 2.1 binaries. Release note: None 26977: storage: factor out node dialer from RaftTransport r=bdarnell a=tschottdorf RaftTransport wraps RPC connections between nodes with additional circuit breaker logic to avoid spamming dead nodes (plus spamming the logs). Upcoming work on follower reads needs similar machinery, and there are various other components in the system that connect to other nodes and should already be using such a mechanism. This commit extracts this logic into a standalone component named `NodeDialer`. In future work, a single `NodeDialer` will be shared across a running node. There is a semantic change in behavior that deserves being called out. Prior to this change, the Raft circuit breakers were tripped not only based on whether it was possible to connect to a given target node, but also based on error codes returned from the calls, and notably StoreNotFoundError. This is no longer the case, as it would be at odds with the goal of sharing these circuit breakers across multiple use cases and maintaining the old behavior would require adding another layer of circuit breaking inside the Raft transport. Looking at the history of this behavior (#10266), it seems to have been introduced because bringing a new node up under the address of an old node could wedge the (ancient version of the) transport indefinitely as it didn't reconnect in that case (taking the correct address from Gossip) but kept sending messages to the wrong node. This is still fixed with my changes; only the backoff is lost, which seems acceptable. Release note: None Co-authored-by: Nathan VanBenschoten <[email protected]> Co-authored-by: Tobias Schottdorf <[email protected]>
Build succeeded |
This change removes the compatibility layer introduced with
VersionMeta2Splitsthat conditionally decided whethersplitting meta2 ranges was permitted. In doing so, it cleans
up some plumbing code that is no longer needed in 2.1 binaries.
Release note: None