From 2b9c931ed0ae901ceba3c71c6e0ee89c7a0393a0 Mon Sep 17 00:00:00 2001 From: Ben Darnell Date: Wed, 18 Apr 2018 14:57:48 -0400 Subject: [PATCH] storage: Remove Store.canCampaignIdleReplica With PreVote, it is less disruptive to campaign unnecessarily, so there's no need for this additional check. Release note: None --- pkg/server/node.go | 8 ------ pkg/storage/replica.go | 7 ----- pkg/storage/store.go | 54 +-------------------------------------- pkg/storage/store_test.go | 43 ------------------------------- 4 files changed, 1 insertion(+), 111 deletions(-) diff --git a/pkg/server/node.go b/pkg/server/node.go index fec9f68edc98..f9750b89970e 100644 --- a/pkg/server/node.go +++ b/pkg/server/node.go @@ -412,14 +412,6 @@ func (n *Node) start( return err } - if n.initialBoot { - // The cluster was just bootstrapped by this node, explicitly notify the - // stores that they were bootstrapped. - for _, s := range stores { - s.NotifyBootstrapped() - } - } - if err := n.startStores(ctx, stores, n.stopper); err != nil { return err } diff --git a/pkg/storage/replica.go b/pkg/storage/replica.go index a56620a513e1..09a84cc08bc4 100644 --- a/pkg/storage/replica.go +++ b/pkg/storage/replica.go @@ -541,13 +541,6 @@ func (r *Replica) withRaftGroupLocked( return nil } - if shouldCampaignOnCreation { - // Special handling of idle replicas: we campaign their Raft group upon - // creation if we gossiped our store descriptor more than the election - // timeout in the past. - shouldCampaignOnCreation = (r.mu.internalRaftGroup == nil) && r.store.canCampaignIdleReplica() - } - ctx := r.AnnotateCtx(context.TODO()) if r.mu.internalRaftGroup == nil { diff --git a/pkg/storage/store.go b/pkg/storage/store.go index b45821629b2c..0a33f3617c14 100644 --- a/pkg/storage/store.go +++ b/pkg/storage/store.go @@ -397,11 +397,6 @@ type Store struct { nodeDesc *roachpb.NodeDescriptor initComplete sync.WaitGroup // Signaled by async init tasks - idleReplicaElectionTime struct { - syncutil.Mutex - at time.Time - } - // Semaphore to limit concurrent non-empty snapshot application and replica // data destruction. snapshotApplySem chan struct{} @@ -1543,36 +1538,7 @@ func (s *Store) GossipStore(ctx context.Context) error { // Unique gossip key per store. gossipStoreKey := gossip.MakeStoreKey(storeDesc.StoreID) // Gossip store descriptor. - if err := s.cfg.Gossip.AddInfoProto(gossipStoreKey, storeDesc, gossip.StoreTTL); err != nil { - return err - } - // Once we have gossiped the store descriptor the first time, other nodes - // will know that this node has restarted and will start sending Raft - // heartbeats for active ranges. We compute the time in the future where a - // replica on this store which receives a command for an idle range can - // campaign the associated Raft group immediately instead of waiting for the - // normal Raft election timeout. - // - // Note that computing this timestamp here is conservative. We really care - // that the node descriptor has been gossiped as that is how remote nodes - // locate this one to send Raft messages. The initialization sequence is: - // 1. gossip node descriptor - // 2. wait for gossip to be connected - // 3. gossip store descriptors (where we're at here) - s.idleReplicaElectionTime.Lock() - if s.idleReplicaElectionTime.at == (time.Time{}) { - // Raft uses a randomized election timeout in the range - // [electionTimeout,2*electionTimeout]. Using the lower bound here means - // that elections are somewhat more likely to be contested (assuming - // traffic is distributed evenly across a cluster that is restarted - // simultaneously). That's OK; it just adds a network round trip or two to - // the process since a contested election just restarts the clock to where - // it would have been anyway if we weren't doing idle replica campaigning. - electionTimeout := s.cfg.RaftTickInterval * time.Duration(s.cfg.RaftElectionTimeoutTicks) - s.idleReplicaElectionTime.at = s.Clock().PhysicalTime().Add(electionTimeout) - } - s.idleReplicaElectionTime.Unlock() - return nil + return s.cfg.Gossip.AddInfoProto(gossipStoreKey, storeDesc, gossip.StoreTTL) } type capacityChangeEvent int @@ -1613,15 +1579,6 @@ func (s *Store) recordNewWritesPerSecond(newVal float64) { } } -func (s *Store) canCampaignIdleReplica() bool { - s.idleReplicaElectionTime.Lock() - defer s.idleReplicaElectionTime.Unlock() - if s.idleReplicaElectionTime.at == (time.Time{}) { - return false - } - return !s.Clock().PhysicalTime().Before(s.idleReplicaElectionTime.at) -} - // GossipDeadReplicas broadcasts the store's dead replicas on the gossip // network. func (s *Store) GossipDeadReplicas(ctx context.Context) error { @@ -1674,7 +1631,6 @@ func (s *Store) Bootstrap( return errors.Wrap(err, "persisting bootstrap data") } - s.NotifyBootstrapped() return nil } @@ -1795,14 +1751,6 @@ func checkEngineEmpty(ctx context.Context, eng engine.Engine) error { return nil } -// NotifyBootstrapped tells the store that it was bootstrapped and allows idle -// replicas to campaign immediately. This primarily affects tests. -func (s *Store) NotifyBootstrapped() { - s.idleReplicaElectionTime.Lock() - s.idleReplicaElectionTime.at = s.Clock().PhysicalTime() - s.idleReplicaElectionTime.Unlock() -} - // GetReplica fetches a replica by Range ID. Returns an error if no replica is found. func (s *Store) GetReplica(rangeID roachpb.RangeID) (*Replica, error) { if value, ok := s.mu.replicas.Load(int64(rangeID)); ok { diff --git a/pkg/storage/store_test.go b/pkg/storage/store_test.go index 1924ddf23f62..31ca16c7f2ed 100644 --- a/pkg/storage/store_test.go +++ b/pkg/storage/store_test.go @@ -2868,49 +2868,6 @@ func TestRemovedReplicaTombstone(t *testing.T) { } } -func TestCanCampaignIdleReplica(t *testing.T) { - defer leaktest.AfterTest(t)() - tc := testContext{} - stopper := stop.NewStopper() - defer stopper.Stop(context.TODO()) - tc.Start(t, stopper) - s := tc.store - ctx := context.Background() - - s.idleReplicaElectionTime.Lock() - s.idleReplicaElectionTime.at = time.Time{} - s.idleReplicaElectionTime.Unlock() - - // Bump the clock by the election timeout. Idle replicas can't campaign - // eagerly yet because we haven't gossiped the store descriptor. - electionTimeout := int64(s.cfg.RaftTickInterval * time.Duration(s.cfg.RaftElectionTimeoutTicks)) - tc.manualClock.Increment(electionTimeout) - if s.canCampaignIdleReplica() { - t.Fatalf("idle replica can unexpectedly campaign") - } - - // Gossip the store descriptor. - if err := s.GossipStore(ctx); err != nil { - t.Fatal(err) - } - if s.canCampaignIdleReplica() { - t.Fatalf("idle replica can unexpectedly campaign") - } - - // Bump the clock to just before the idle election time. - tc.manualClock.Increment(electionTimeout - 1) - if s.canCampaignIdleReplica() { - t.Fatalf("idle replica can unexpectedly campaign") - } - - // One more nanosecond bump and idle replicas should be able to campaign - // eagerly. - tc.manualClock.Increment(1) - if !s.canCampaignIdleReplica() { - t.Fatalf("idle replica unexpectedly cannot campaign") - } -} - type fakeSnapshotStream struct { nextResp *SnapshotResponse nextErr error