Skip to content

Commit

Permalink
kvserver: get rid of bespoke relocation logic used by the mergeQueue
Browse files Browse the repository at this point in the history 
This commit removes the relocation logic used by the `mergeQueue` thus
far to align replica sets (added in cockroachdb#56197). This logic previously
existed in order to allow us to align the replica sets of a pair of
ranges (which is required for the range merge to proceed), while
avoiding redundant data movement.

Before cockroachdb#58627 and the previous commit in this PR, `AdminRelocateRange`
couldn't be directly used by the mergeQueue under various configurations
of the LHS and RHS ranges. Furthermore, even when it could be used, it
would involve redundant data movement. This all required us to compute
relocation targets for the RHS of a merge separately, above the call to
`AdminRelocateRange`, for the range merge to proceed.

All these limitations have been resolved by the previous commit which
teaches `AdminRelocateRange` to promote non-voters and demote voters
when needed, and the aforementioned bespoke relocation logic is no
longer needed.

Release note: None
  • Loading branch information
@aayushshah15
aayushshah15 committed Mar 28, 2021
1 parent f460ce6 commit 74b6ec9
Show file tree
Hide file tree
Showing 7 changed files with 108 additions and 234 deletions.
1 change: 0 additions & 1 deletion pkg/kv/kvserver/BUILD.bazel
View file
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,6 @@ go_library(
visibility = ["//visibility:public"],
deps = [
"//pkg/base",
"//pkg/build",
"//pkg/clusterversion",
"//pkg/config",
"//pkg/config/zonepb",
Expand Down
197 changes: 79 additions & 118 deletions pkg/kv/kvserver/client_merge_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ import (
"math/rand"
"reflect"
"regexp"
"sort"
"strings"
"sync"
"sync/atomic"
Expand Down Expand Up @@ -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},
},
}

Expand Down Expand Up @@ -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)
Expand Down
17 changes: 15 additions & 2 deletions pkg/kv/kvserver/client_relocate_range_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import (
"math/rand"
"sort"
"testing"
"time"

"github.com/cockroachdb/cockroach/pkg/base"
"github.com/cockroachdb/cockroach/pkg/keys"
Expand Down Expand Up @@ -277,10 +278,22 @@ func TestAdminRelocateRangeRandom(t *testing.T) {

args := base.TestClusterArgs{
ReplicationMode: base.ReplicationManual,
ServerArgs: base.TestServerArgs{
Knobs: base.TestingKnobs{
Store: &kvserver.StoreTestingKnobs{
DontIgnoreFailureToTransferLease: true,
},
NodeLiveness: kvserver.NodeLivenessTestingKnobs{
// Use a long liveness duration to avoid flakiness under stress on the
// lease check performed by `relocateAndCheck`.
LivenessDuration: 20 * time.Second,
},
},
},
}
numNodes, numIterations := 9, 10
numNodes, numIterations := 5, 10
if util.RaceEnabled {
numNodes, numIterations = 4, 1
numNodes, numIterations = 3, 1
}

randomRelocationTargets := func() (voterTargets, nonVoterTargets []int) {
Expand Down
6 changes: 2 additions & 4 deletions pkg/kv/kvserver/merge_queue.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
Loading

0 comments on commit 74b6ec9

Please sign in to comment.