Skip to content

Commit

Permalink
kvclient: remove unused MoveToFront
Browse files Browse the repository at this point in the history
This commit removes ths MoveToFront method on the Transport. A major
advantage of this change is that the Transport no longer modifies the
underlying ReplicaSlice. This will allow a future commit to avoid
copying the slice to every transport.

Epic: none

Release note: None
  • Loading branch information
andrewbaptist committed Jan 19, 2024
1 parent fa990c1 commit 801664f
Show file tree
Hide file tree
Showing 6 changed files with 0 additions and 145 deletions.
4 changes: 0 additions & 4 deletions pkg/kv/kvclient/kvcoord/dist_sender_rangefeed_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -157,10 +157,6 @@ func (c *countConnectionsTransport) SkipReplica() {
c.wrapped.SkipReplica()
}

func (c *countConnectionsTransport) MoveToFront(descriptor roachpb.ReplicaDescriptor) bool {
return c.wrapped.MoveToFront(descriptor)
}

func (c *countConnectionsTransport) Release() {
c.wrapped.Release()
}
Expand Down
16 changes: 0 additions & 16 deletions pkg/kv/kvclient/kvcoord/dist_sender_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -198,22 +198,6 @@ func (l *simpleTransportAdapter) SkipReplica() {
l.nextReplicaIdx++
}

func (l *simpleTransportAdapter) MoveToFront(replica roachpb.ReplicaDescriptor) bool {
for i := range l.replicas {
if l.replicas[i].IsSame(replica) {
// If we've already processed the replica, decrement the current
// index before we swap.
if i < l.nextReplicaIdx {
l.nextReplicaIdx--
}
// Swap the client representing this replica to the front.
l.replicas[i], l.replicas[l.nextReplicaIdx] = l.replicas[l.nextReplicaIdx], l.replicas[i]
return true
}
}
return false
}

func (l *simpleTransportAdapter) Release() {}

func makeGossip(t *testing.T, stopper *stop.Stopper, rpcContext *rpc.Context) *gossip.Gossip {
Expand Down
14 changes: 0 additions & 14 deletions pkg/kv/kvclient/kvcoord/mocks_generated_test.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 0 additions & 4 deletions pkg/kv/kvclient/kvcoord/send_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -189,10 +189,6 @@ func (f *firstNErrorTransport) SkipReplica() {
panic("SkipReplica not supported")
}

func (*firstNErrorTransport) MoveToFront(roachpb.ReplicaDescriptor) bool {
return true
}

// TestComplexScenarios verifies various complex success/failure scenarios by
// mocking sendOne.
func TestComplexScenarios(t *testing.T) {
Expand Down
27 changes: 0 additions & 27 deletions pkg/kv/kvclient/kvcoord/transport.go
Original file line number Diff line number Diff line change
Expand Up @@ -78,13 +78,6 @@ type Transport interface {
// - the replica that NextReplica() would return is skipped.
SkipReplica()

// MoveToFront locates the specified replica and moves it to the
// front of the ordering of replicas to try. If the replica has
// already been tried, it will be retried. Returns false if the specified
// replica can't be found and thus can't be moved to the front of the
// transport.
MoveToFront(roachpb.ReplicaDescriptor) bool

// Release releases any resources held by this Transport.
Release()
}
Expand Down Expand Up @@ -267,22 +260,6 @@ func (gt *grpcTransport) SkipReplica() {
gt.nextReplicaIdx++
}

func (gt *grpcTransport) MoveToFront(replica roachpb.ReplicaDescriptor) bool {
for i := range gt.replicas {
if gt.replicas[i].IsSame(replica) {
// If we've already processed the replica, decrement the current
// index before we swap.
if i < gt.nextReplicaIdx {
gt.nextReplicaIdx--
}
// Swap the client representing this replica to the front.
gt.replicas[i], gt.replicas[gt.nextReplicaIdx] = gt.replicas[gt.nextReplicaIdx], gt.replicas[i]
return true
}
}
return false
}

// splitHealthy splits the grpcTransport's replica slice into healthy replica
// and unhealthy replica, based on their connection state. Healthy replicas will
// be rearranged first in the replicas slice, and unhealthy replicas will be
Expand Down Expand Up @@ -390,8 +367,4 @@ func (s *senderTransport) SkipReplica() {
s.called = true
}

func (s *senderTransport) MoveToFront(replica roachpb.ReplicaDescriptor) bool {
return true
}

func (s *senderTransport) Release() {}
80 changes: 0 additions & 80 deletions pkg/kv/kvclient/kvcoord/transport_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ import (
"github.com/cockroachdb/cockroach/pkg/kv/kvpb"
"github.com/cockroachdb/cockroach/pkg/roachpb"
"github.com/cockroachdb/cockroach/pkg/testutils"
"github.com/cockroachdb/cockroach/pkg/util/caller"
"github.com/cockroachdb/cockroach/pkg/util/leaktest"
"github.com/cockroachdb/cockroach/pkg/util/log"
"github.com/cockroachdb/cockroach/pkg/util/tracing"
Expand All @@ -27,85 +26,6 @@ import (
"google.golang.org/grpc"
)

func TestTransportMoveToFront(t *testing.T) {
defer leaktest.AfterTest(t)()
defer log.Scope(t).Close(t)
rd1 := roachpb.ReplicaDescriptor{NodeID: 1, StoreID: 1, ReplicaID: 1}
rd2 := roachpb.ReplicaDescriptor{NodeID: 2, StoreID: 2, ReplicaID: 2}
rd3 := roachpb.ReplicaDescriptor{NodeID: 3, StoreID: 3, ReplicaID: 3}
rd3Incoming := roachpb.ReplicaDescriptor{NodeID: 3, StoreID: 3, ReplicaID: 3,
Type: roachpb.VOTER_INCOMING}
gt := grpcTransport{replicas: []roachpb.ReplicaDescriptor{rd1, rd2, rd3}}

verifyOrder := func(replicas []roachpb.ReplicaDescriptor) {
file, line, _ := caller.Lookup(1)
for i, r := range gt.replicas {
if r != replicas[i] {
t.Fatalf("%s:%d: expected order %+v; got mismatch at index %d: %+v",
file, line, replicas, i, r)
}
}
}

verifyOrder([]roachpb.ReplicaDescriptor{rd1, rd2, rd3})

// Move replica 2 to the front.
gt.MoveToFront(rd2)
verifyOrder([]roachpb.ReplicaDescriptor{rd2, rd1, rd3})

// Now replica 3.
gt.MoveToFront(rd3)
verifyOrder([]roachpb.ReplicaDescriptor{rd3, rd1, rd2})

// Advance the client index and move replica 3 back to front.
gt.nextReplicaIdx++
gt.MoveToFront(rd3)
verifyOrder([]roachpb.ReplicaDescriptor{rd3, rd1, rd2})
if gt.nextReplicaIdx != 0 {
t.Fatalf("expected client index 0; got %d", gt.nextReplicaIdx)
}

// Advance the client index again and verify replica 3 can
// be moved to front for a second retry.
gt.nextReplicaIdx++
gt.MoveToFront(rd3)
verifyOrder([]roachpb.ReplicaDescriptor{rd3, rd1, rd2})
if gt.nextReplicaIdx != 0 {
t.Fatalf("expected client index 0; got %d", gt.nextReplicaIdx)
}

// Move replica 2 to the front.
gt.MoveToFront(rd2)
verifyOrder([]roachpb.ReplicaDescriptor{rd2, rd1, rd3})

// Advance client index and move rd1 front; should be no change.
gt.nextReplicaIdx++
gt.MoveToFront(rd1)
verifyOrder([]roachpb.ReplicaDescriptor{rd2, rd1, rd3})

// Advance client index and and move rd1 to front. Should move
// client index back for a retry.
gt.nextReplicaIdx++
gt.MoveToFront(rd1)
verifyOrder([]roachpb.ReplicaDescriptor{rd2, rd1, rd3})
if gt.nextReplicaIdx != 1 {
t.Fatalf("expected client index 1; got %d", gt.nextReplicaIdx)
}

// Advance client index once more; verify second retry.
gt.nextReplicaIdx++
gt.MoveToFront(rd2)
verifyOrder([]roachpb.ReplicaDescriptor{rd1, rd2, rd3})
if gt.nextReplicaIdx != 1 {
t.Fatalf("expected client index 1; got %d", gt.nextReplicaIdx)
}

// Move rd3 to front, even if the replica type differs.
gt.MoveToFront(rd3Incoming)
verifyOrder([]roachpb.ReplicaDescriptor{rd1, rd3, rd2})
require.Equal(t, 1, gt.nextReplicaIdx)
}

// TestSpanImport tests that the gRPC transport ingests trace information that
// came from gRPC responses (via tracingpb.RecordedSpan on the batch responses).
func TestSpanImport(t *testing.T) {
Expand Down

0 comments on commit 801664f

Please sign in to comment.