diff --git a/pkg/kv/kvserver/BUILD.bazel b/pkg/kv/kvserver/BUILD.bazel index c3593c73ea99..c0e4c92118ec 100644 --- a/pkg/kv/kvserver/BUILD.bazel +++ b/pkg/kv/kvserver/BUILD.bazel @@ -99,7 +99,6 @@ go_library( visibility = ["//visibility:public"], deps = [ "//pkg/base", - "//pkg/build", "//pkg/clusterversion", "//pkg/config", "//pkg/config/zonepb", diff --git a/pkg/kv/kvserver/client_merge_test.go b/pkg/kv/kvserver/client_merge_test.go index 8feb60467878..a104540db7ed 100644 --- a/pkg/kv/kvserver/client_merge_test.go +++ b/pkg/kv/kvserver/client_merge_test.go @@ -18,7 +18,6 @@ import ( "math/rand" "reflect" "regexp" - "sort" "strings" "sync" "sync/atomic" @@ -4175,137 +4174,117 @@ func TestMergeQueueSeesNonVoters(t *testing.T) { type test struct { name string leftVoters, rightVoters, leftNonVoters, rightNonVoters []int - expectedRightVoters, expectedRightNonVoters []int } // NB: The test setup code places a single voter replica on (n1,s1) for both // left and right range, which we remove after setting the test up. tests := []test{ { - name: "collocated-per-type", - leftVoters: []int{2, 3, 4}, - rightVoters: []int{2, 3, 4}, - leftNonVoters: []int{1}, - rightNonVoters: []int{1}, - expectedRightVoters: []int{2, 3, 4}, - expectedRightNonVoters: []int{1}, + name: "collocated-per-type", + leftVoters: []int{2, 3, 4}, + rightVoters: []int{2, 3, 4}, + leftNonVoters: []int{1}, + rightNonVoters: []int{1}, }, { - name: "collocated-overall", - leftVoters: []int{3, 4}, - rightVoters: []int{1, 2}, - leftNonVoters: []int{1, 2}, - rightNonVoters: []int{3, 4}, - expectedRightVoters: []int{1, 2}, - expectedRightNonVoters: []int{3, 4}, + name: "collocated-overall", + leftVoters: []int{3, 4}, + rightVoters: []int{1, 2}, + leftNonVoters: []int{1, 2}, + rightNonVoters: []int{3, 4}, }, { - name: "collocated-voters-only", - leftVoters: []int{3, 4}, - rightVoters: []int{3, 4}, - leftNonVoters: []int{2}, - rightNonVoters: []int{1}, - expectedRightVoters: []int{3, 4}, - expectedRightNonVoters: []int{2}, + name: "collocated-voters-only", + leftVoters: []int{3, 4}, + rightVoters: []int{3, 4}, + leftNonVoters: []int{2}, + rightNonVoters: []int{1}, }, { - name: "collocated-non-voters-only", - leftVoters: []int{3}, - rightVoters: []int{4}, - leftNonVoters: []int{1, 2}, - rightNonVoters: []int{1, 2}, - expectedRightVoters: []int{3}, - expectedRightNonVoters: []int{1, 2}, + name: "collocated-non-voters-only", + leftVoters: []int{3}, + rightVoters: []int{4}, + leftNonVoters: []int{1, 2}, + rightNonVoters: []int{1, 2}, }, { - name: "not-collocated", - leftVoters: []int{3}, - rightVoters: []int{4}, - leftNonVoters: []int{2}, - rightNonVoters: []int{1}, - expectedRightVoters: []int{3}, - expectedRightNonVoters: []int{2}, + name: "not-collocated", + leftVoters: []int{3}, + rightVoters: []int{4}, + leftNonVoters: []int{2}, + rightNonVoters: []int{1}, }, { - name: "partially-collocated-voters-only", - leftVoters: []int{2, 3}, - rightVoters: []int{1, 4}, - leftNonVoters: []int{1}, - rightNonVoters: []int{2}, - expectedRightVoters: []int{1, 3}, - expectedRightNonVoters: []int{2}, + name: "partially-collocated-voters-only", + leftVoters: []int{2, 3}, + rightVoters: []int{1, 4}, + leftNonVoters: []int{1}, + rightNonVoters: []int{2}, }, { - name: "partially-collocated-non-voters-only", - leftVoters: []int{4}, - rightVoters: []int{4}, - leftNonVoters: []int{1, 3}, - rightNonVoters: []int{1, 2}, - expectedRightVoters: []int{4}, - expectedRightNonVoters: []int{1, 3}, + name: "partially-collocated-non-voters-only", + leftVoters: []int{4}, + rightVoters: []int{4}, + leftNonVoters: []int{1, 3}, + rightNonVoters: []int{1, 2}, }, { - name: "partially-collocated", - leftVoters: []int{2}, - rightVoters: []int{4}, - leftNonVoters: []int{1, 3}, - rightNonVoters: []int{1, 2}, - expectedRightVoters: []int{3}, - expectedRightNonVoters: []int{1, 2}, + name: "partially-collocated", + leftVoters: []int{2}, + rightVoters: []int{4}, + leftNonVoters: []int{1, 3}, + rightNonVoters: []int{1, 2}, }, { - name: "collocated-rhs-being-reconfigured-1", - leftVoters: []int{1, 2, 3}, - rightVoters: []int{1, 2, 3, 4, 5, 6}, - leftNonVoters: []int{4, 5, 6}, - rightNonVoters: []int{}, - expectedRightVoters: []int{1, 2, 3, 4, 5, 6}, - expectedRightNonVoters: []int{}, + name: "collocated-rhs-being-reconfigured-1", + leftVoters: []int{1, 2, 3}, + rightVoters: []int{1, 2, 3, 4, 5, 6}, + leftNonVoters: []int{4, 5, 6}, + rightNonVoters: []int{}, }, { - name: "collocated-rhs-being-reconfigured-2", - leftVoters: []int{1, 2, 3}, - rightVoters: []int{1, 2, 3, 4}, - leftNonVoters: []int{4, 5, 6}, - rightNonVoters: []int{}, - expectedRightVoters: []int{1, 2, 3, 4}, - expectedRightNonVoters: []int{5, 6}, + name: "collocated-rhs-being-reconfigured-2", + leftVoters: []int{1, 2, 3}, + rightVoters: []int{1, 2, 3, 4}, + leftNonVoters: []int{4, 5, 6}, + rightNonVoters: []int{}, }, { - name: "collocated-rhs-being-reconfigured-3", - leftVoters: []int{1, 2, 3}, - rightVoters: []int{1}, - leftNonVoters: []int{4, 5, 6}, - rightNonVoters: []int{2, 3, 4, 5, 6}, - expectedRightVoters: []int{1}, - expectedRightNonVoters: []int{2, 3, 4, 5, 6}, + name: "collocated-rhs-being-reconfigured-3", + leftVoters: []int{1, 2, 3}, + rightVoters: []int{1}, + leftNonVoters: []int{4, 5, 6}, + rightNonVoters: []int{2, 3, 4, 5, 6}, }, { - name: "non-collocated-rhs-being-reconfigured", - leftVoters: []int{1, 2, 3}, - rightVoters: []int{5}, - leftNonVoters: []int{4, 6}, - rightNonVoters: []int{}, - expectedRightVoters: []int{1, 2, 3}, - expectedRightNonVoters: []int{4, 6}, + name: "non-collocated-rhs-being-reconfigured", + leftVoters: []int{1, 2, 3}, + rightVoters: []int{5}, + leftNonVoters: []int{4, 6}, + rightNonVoters: []int{}, }, { - name: "partially-collocated-rhs-being-downreplicated", - leftVoters: []int{1, 2, 3}, - rightVoters: []int{1, 2, 3, 4, 5, 6}, - leftNonVoters: []int{4, 5}, - rightNonVoters: []int{}, - expectedRightVoters: []int{1, 2, 3, 4, 5}, - expectedRightNonVoters: []int{}, + name: "partially-collocated-rhs-being-downreplicated", + leftVoters: []int{1, 2, 3}, + rightVoters: []int{1, 2, 3, 4, 5, 6}, + leftNonVoters: []int{4, 5}, + rightNonVoters: []int{}, }, { - name: "partially-collocated-rhs-being-upreplicated", - leftVoters: []int{1, 2, 3}, - rightVoters: []int{1}, - leftNonVoters: []int{4, 5, 6}, - rightNonVoters: []int{}, - expectedRightVoters: []int{1, 2, 3}, - expectedRightNonVoters: []int{4, 5, 6}, + name: "partially-collocated-rhs-being-upreplicated", + leftVoters: []int{1, 2, 3}, + rightVoters: []int{1}, + leftNonVoters: []int{4, 5, 6}, + rightNonVoters: []int{}, + }, + { + // This is a subtest that should trigger at least 3 voter<->non-voter + // swaps. + name: "lhs-voters-collocated-with-rhs-non-voters", + leftVoters: []int{1, 2, 3}, + rightVoters: []int{4}, + leftNonVoters: []int{}, + rightNonVoters: []int{1, 2, 3}, }, } @@ -4374,24 +4353,6 @@ func TestMergeQueueSeesNonVoters(t *testing.T) { tc.RemoveVotersOrFatal(t, rightDesc.StartKey.AsRawKey(), tc.Target(0)) rightDesc = tc.LookupRangeOrFatal(t, rightDesc.StartKey.AsRawKey()) - // Check that we're avoiding superfluous data movement. - voterTargets, nonVoterTargets, err := kvserver.GetTargetsToCollocateRHSForMerge(ctx, leftDesc.Replicas(), rightDesc.Replicas()) - require.NoError(t, err) - require.Equal(t, len(subtest.expectedRightVoters), len(voterTargets)) - require.Equal(t, len(subtest.expectedRightNonVoters), len(nonVoterTargets)) - sort.Slice(voterTargets, func(i, j int) bool { - return voterTargets[i].NodeID < voterTargets[j].NodeID - }) - sort.Slice(nonVoterTargets, func(i, j int) bool { - return nonVoterTargets[i].NodeID < nonVoterTargets[j].NodeID - }) - for i := range subtest.expectedRightVoters { - require.Equal(t, tc.Target(subtest.expectedRightVoters[i]), voterTargets[i]) - } - for i := range subtest.expectedRightNonVoters { - require.Equal(t, tc.Target(subtest.expectedRightNonVoters[i]), nonVoterTargets[i]) - } - store.SetMergeQueueActive(true) store.MustForceMergeScanAndProcess() verifyMerged(t, store, leftDesc.StartKey, rightDesc.StartKey) diff --git a/pkg/kv/kvserver/merge_queue.go b/pkg/kv/kvserver/merge_queue.go index 924d7e425dee..81c4bdd57078 100644 --- a/pkg/kv/kvserver/merge_queue.go +++ b/pkg/kv/kvserver/merge_queue.go @@ -304,10 +304,8 @@ func (mq *mergeQueue) process( // these ranges and only try to collocate them if they're not in violation, // which would help us make better guarantees about not transiently // violating constraints during a merge. - voterTargets, nonVoterTargets, err := GetTargetsToCollocateRHSForMerge(ctx, lhsDesc.Replicas(), rhsDesc.Replicas()) - if err != nil { - return false, err - } + voterTargets := lhsDesc.Replicas().Voters().ReplicationTargets() + nonVoterTargets := lhsDesc.Replicas().NonVoters().ReplicationTargets() // AdminRelocateRange moves the lease to the first target in the list, so // sort the existing leaseholder there to leave it unchanged. diff --git a/pkg/kv/kvserver/replica_command.go b/pkg/kv/kvserver/replica_command.go index 3dc2698db28d..51810adadac4 100644 --- a/pkg/kv/kvserver/replica_command.go +++ b/pkg/kv/kvserver/replica_command.go @@ -20,7 +20,6 @@ import ( "time" "github.com/cockroachdb/cockroach/pkg/base" - "github.com/cockroachdb/cockroach/pkg/build" "github.com/cockroachdb/cockroach/pkg/clusterversion" "github.com/cockroachdb/cockroach/pkg/keys" "github.com/cockroachdb/cockroach/pkg/kv" @@ -2467,100 +2466,6 @@ func replicasCollocated(a, b []roachpb.ReplicaDescriptor) bool { return true } -// GetTargetsToCollocateRHSForMerge decides the configuration of RHS replicas -// need before the rhs can be subsumed and then merged into the LHS range. The -// desired RHS voters and non-voters are returned; together they'll cover the -// same stores as LHS's replicas, but the configuration of replicas doesn't -// necessarily match (it doesn't need to match for the merge). -// -// We compute the new voter / non-voter targets for the RHS by first -// bootstrapping our result set with the replicas that are already collocated. -// We then step through RHS's non-collocated voters and try to move them to -// stores that already have a voter for LHS. If this is not possible for all the -// non-collocated voters of RHS (i.e. because the RHS has non-voter(s) on -// store(s) where the LHS has voter(s)), we may move some RHS voters to targets -// that have non-voters for LHS. Likewise, we do the same for the non-collocated -// non-voters of RHS: try to relocate them to stores where the LHS has -// non-voters, but resort to relocating them to stores where the LHS has voters. -// -// TODO(aayush): Can moving a voter replica from RHS to a store that has a -// non-voter for LHS (or vice versa) can lead to constraint violations? Justify -// why or why not. -func GetTargetsToCollocateRHSForMerge( - ctx context.Context, leftRepls, rightRepls roachpb.ReplicaSet, -) (voterTargets, nonVoterTargets []roachpb.ReplicationTarget, _ error) { - notInRight := func(desc roachpb.ReplicaDescriptor) bool { - return !rightRepls.Contains(desc) - } - - // Sets of replicas that exist on the LHS but not on the RHS - leftMinusRight := leftRepls.Filter(notInRight) - leftMinusRightVoters := leftMinusRight.Voters().Descriptors() - leftMinusRightNonVoters := leftMinusRight.NonVoters().Descriptors() - - // We bootstrap our result set by first including the replicas (voting and - // non-voting) that _are_ collocated, as these will stay unchanged and will - // be no-ops when passed through AdminRelocateRange. - finalRightVoters := rightRepls.Voters().Filter(leftRepls.Contains).DeepCopy() - finalRightNonVoters := rightRepls.NonVoters().Filter(leftRepls.Contains).DeepCopy() - - needMore := func() bool { - return len(finalRightVoters.Descriptors())+len(finalRightNonVoters.Descriptors()) < len(leftRepls.Descriptors()) - } - - numVoters := len(leftRepls.VoterDescriptors()) - // We loop through the set of non-collocated replicas and figure out a - // suitable configuration to relocate RHS's replicas to. At the end of these - // two loops, we will have exhausted `leftMinusRight`. - for len(finalRightVoters.Descriptors()) < numVoters && needMore() { - // Prefer to relocate voters for RHS to stores that have voters for LHS, but - // resort to relocating them to stores with non-voters for LHS if that's not - // possible. - if len(leftMinusRightVoters) != 0 { - finalRightVoters.AddReplica(leftMinusRightVoters[0]) - leftMinusRightVoters = leftMinusRightVoters[1:] - } else if len(leftMinusRightNonVoters) != 0 { - finalRightVoters.AddReplica(leftMinusRightNonVoters[0]) - leftMinusRightNonVoters = leftMinusRightNonVoters[1:] - } else { - log.Fatalf(ctx, "programming error: unexpectedly ran out of valid stores to relocate RHS"+ - " voters to; LHS: %s, RHS: %s", leftRepls.Descriptors(), rightRepls.Descriptors()) - } - } - - for needMore() { - // Like above, we try to relocate non-voters for RHS to stores that have - // non-voters for LHS, but resort to relocating them to stores with voters - // for LHS if that's not possible. - if len(leftMinusRightNonVoters) != 0 { - finalRightNonVoters.AddReplica(leftMinusRightNonVoters[0]) - leftMinusRightNonVoters = leftMinusRightNonVoters[1:] - } else if len(leftMinusRightVoters) != 0 { - finalRightNonVoters.AddReplica(leftMinusRightVoters[0]) - leftMinusRightVoters = leftMinusRightVoters[1:] - } else { - log.Fatalf(ctx, "programming error: unexpectedly ran out of valid stores to relocate RHS"+ - " non-voters to; LHS: %s, RHS: %s", leftRepls.Descriptors(), rightRepls.Descriptors()) - } - } - - if len(finalRightVoters.Descriptors()) == 0 { - // TODO(aayush): We can end up in this case for scenarios like the - // following (the digits represent StoreIDs): - // - // LHS-> voters: {1, 2, 3}, non-voters: {} - // RHS-> voters: {4}, non-voters: {1, 2, 3} - // - // Remove this error path once we support swapping voters and non-voters. - return nil, nil, - errors.UnimplementedErrorf(errors.IssueLink{IssueURL: build.MakeIssueURL(58499)}, - "unsupported configuration of RHS(%s) and LHS(%s) as it requires an atomic swap of a"+ - " voter and non-voter", rightRepls, leftRepls) - } - - return finalRightVoters.ReplicationTargets(), finalRightNonVoters.ReplicationTargets(), nil -} - func checkDescsEqual(desc *roachpb.RangeDescriptor) func(*roachpb.RangeDescriptor) bool { return func(desc2 *roachpb.RangeDescriptor) bool { return desc.Equal(desc2) diff --git a/pkg/roachpb/metadata_replicas.go b/pkg/roachpb/metadata_replicas.go index cae6035c4c2d..77e61e9bad9e 100644 --- a/pkg/roachpb/metadata_replicas.go +++ b/pkg/roachpb/metadata_replicas.go @@ -334,18 +334,6 @@ func (d ReplicaSet) DeepCopy() ReplicaSet { } } -// Contains returns true if the set contains rDesc. -func (d ReplicaSet) Contains(rDesc ReplicaDescriptor) bool { - descs := d.Descriptors() - for i := range descs { - repl := &descs[i] - if repl.StoreID == rDesc.StoreID && repl.NodeID == rDesc.NodeID { - return true - } - } - return false -} - // AddReplica adds the given replica to this set. func (d *ReplicaSet) AddReplica(r ReplicaDescriptor) { d.wrapped = append(d.wrapped, r)