Skip to content

Commit

Permalink
kvserver: improve a test
Browse files Browse the repository at this point in the history
This test was performing a configuration change by talking directly to
one of the range's replicas. Instead, this change switches it to use an
Admin request which gets routed to the leaseholder. It's better if the
leaseholder coordinates the configuration change; a non-leaseholder can
error out because it might try to send snapshot before it applied the
preceding descriptor change locally (see cockroachdb#56596). The test seems to
currently work realiably, but, without this patch, becomes flaky with
the next commit which restricts who can take the lease for a range with
no lease.

Release note: None
  • Loading branch information
andreimatei committed Dec 10, 2020
1 parent 00798f9 commit 3c2656b
Show file tree
Hide file tree
Showing 2 changed files with 6 additions and 27 deletions.
15 changes: 6 additions & 9 deletions pkg/kv/kvserver/client_raft_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1166,6 +1166,7 @@ func TestReplicateAfterRemoveAndSplit(t *testing.T) {
defer leaktest.AfterTest(t)()
defer log.Scope(t).Close(t)

ctx := context.Background()
sc := kvserver.TestStoreConfig(nil)
sc.TestingKnobs.DisableMergeQueue = true
sc.TestingKnobs.DisableReplicateQueue = true
Expand Down Expand Up @@ -1201,11 +1202,11 @@ func TestReplicateAfterRemoveAndSplit(t *testing.T) {
// Split the range.
splitKey := roachpb.Key("m")
splitArgs := adminSplitArgs(splitKey)
if _, err := rep1.AdminSplit(context.Background(), *splitArgs, "test"); err != nil {
if _, err := rep1.AdminSplit(ctx, *splitArgs, "test"); err != nil {
t.Fatal(err)
}

mtc.advanceClock(context.Background())
mtc.advanceClock(ctx)

// Restart store 2.
mtc.restartStore(2)
Expand All @@ -1217,20 +1218,16 @@ func TestReplicateAfterRemoveAndSplit(t *testing.T) {
startKey := roachpb.RKey(splitKey)

var desc roachpb.RangeDescriptor
if err := mtc.dbs[0].GetProto(context.Background(), keys.RangeDescriptorKey(startKey), &desc); err != nil {
t.Fatal(err)
}

rep2, err := mtc.findMemberStoreLocked(desc).GetReplica(desc.RangeID)
if err != nil {
if err := mtc.dbs[0].GetProto(ctx, keys.RangeDescriptorKey(startKey), &desc); err != nil {
t.Fatal(err)
}

chgs := roachpb.MakeReplicationChanges(roachpb.ADD_REPLICA, roachpb.ReplicationTarget{
NodeID: mtc.stores[2].Ident.NodeID,
StoreID: mtc.stores[2].Ident.StoreID,
})
_, err = rep2.ChangeReplicas(context.Background(), &desc, kvserver.SnapshotRequest_REBALANCE, kvserverpb.ReasonRangeUnderReplicated, "", chgs)

_, err = mtc.dbs[0].AdminChangeReplicas(ctx, startKey, desc, chgs)
return err
}

Expand Down
18 changes: 0 additions & 18 deletions pkg/kv/kvserver/client_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1157,24 +1157,6 @@ func (m *multiTestContext) findStartKeyLocked(rangeID roachpb.RangeID) roachpb.R
return nil // unreached, but the compiler can't tell.
}

// findMemberStoreLocked finds a non-stopped Store which is a member
// of the given range.
func (m *multiTestContext) findMemberStoreLocked(desc roachpb.RangeDescriptor) *kvserver.Store {
for _, s := range m.stores {
if s == nil {
// Store is stopped.
continue
}
for _, r := range desc.InternalReplicas {
if s.StoreID() == r.StoreID {
return s
}
}
}
m.t.Fatalf("couldn't find a live member of %s", &desc)
return nil // unreached, but the compiler can't tell.
}

// restart stops and restarts all stores but leaves the engines intact,
// so the stores should contain the same persistent storage as before.
func (m *multiTestContext) restart() {
Expand Down

0 comments on commit 3c2656b

Please sign in to comment.