Merge #88472 #88482 #88486

88472: gossip: Don't check if the network should be tightened so often r=kvoli,nvanbenschoten a=a-robinson

Touches #51838. See the thread there for more detail, but the gist is that tightenNetwork gets called hundreds of times per second (taking a full mutex lock each time) on large clusters, even when nothing needs tightening.

Release note (performance improvement): Avoid wasteful contention on the gossip mutex caused by checking if the network needs tightening hundreds of times per second.

88482: kv: remove ConditionalPutRequest.DeprecatedExpValue r=AlexTalks a=nvanbenschoten

This commit removes the ConditionalPutRequest.DeprecatedExpValue field, which was replaced by the ExpBytes introduced in 9e14f24. This has been possible since v21.1.

Release justification: None, wait.

Release note: None.

88486: github: route admission prs to targeted github team r=irfansharif a=irfansharif

Release note: None

Co-authored-by: Alex Robinson <[email protected]>
Co-authored-by: Nathan VanBenschoten <[email protected]>
Co-authored-by: irfan sharif <[email protected]>

.github/CODEOWNERS

@@ -354,8 +354,9 @@
 /pkg/util/addr/              @cockroachdb/cli-prs @cockroachdb/obs-inf-prs
 /pkg/util/metric/            @cockroachdb/obs-inf-prs
 /pkg/util/stop/              @cockroachdb/kv-prs
-/pkg/util/grunning/          @cockroachdb/kv-prs
-/pkg/util/admission/         @cockroachdb/kv-prs
+/pkg/util/grunning/          @cockroachdb/admission-control
+/pkg/util/admission/         @cockroachdb/admission-control
+/pkg/util/schedulerlatency/  @cockroachdb/admission-control
 /pkg/util/tracing            @cockroachdb/obs-inf-prs
 /pkg/workload/               @cockroachdb/sql-experience-noreview
 /pkg/obs/                    @cockroachdb/obs-inf-prs

TEAMS.yaml

@@ -36,6 +36,7 @@ cockroachdb/sql-observability:
   triage_column_id: 12618343
 cockroachdb/kv:
   aliases:
+    cockroachdb/admission-control: other
     cockroachdb/kv-triage: roachtest
     cockroachdb/kv-prs: other
   triage_column_id: 14242655

pkg/gossip/gossip.go

@@ -131,6 +131,10 @@ const (
 	// StoreTTL is time-to-live for store-related info.
 	StoreTTL = 2 * StoresInterval
 
+	// gossipTightenInterval is how long to wait between tightenNetwork checks if
+	// we didn't need to tighten the last time we checked.
+	gossipTightenInterval = time.Second
+
 	unknownNodeID roachpb.NodeID = 0
 )
 
@@ -1462,12 +1466,23 @@ func jitteredInterval(interval time.Duration) time.Duration {
 func (g *Gossip) tightenNetwork(ctx context.Context) {
 	g.mu.Lock()
 	defer g.mu.Unlock()
+
+	now := timeutil.Now()
+	if now.Before(g.mu.lastTighten.Add(gossipTightenInterval)) {
+		// It hasn't been long since we last tightened the network, so skip it.
+		return
+	}
+	g.mu.lastTighten = now
+
 	if g.outgoing.hasSpace() {
 		distantNodeID, distantHops := g.mu.is.mostDistant(g.hasOutgoingLocked)
 		log.VEventf(ctx, 2, "distantHops: %d from %d", distantHops, distantNodeID)
 		if distantHops <= maxHops {
 			return
 		}
+		// If tightening is needed, then reset lastTighten to avoid restricting how
+		// soon we try again.
+		g.mu.lastTighten = time.Time{}
 		if nodeAddr, err := g.getNodeIDAddress(distantNodeID, true /* locked */); err != nil || nodeAddr == nil {
 			log.Health.Errorf(ctx, "unable to get address for n%d: %s", distantNodeID, err)
 		} else {

pkg/gossip/server.go

@@ -55,6 +55,9 @@ type server struct {
 		// composable. There's an open proposal to add them:
 		// https://github.com/golang/go/issues/16620
 		ready chan struct{}
+		// The time at which we last checked if the network should be tightened.
+		// Used to avoid burning CPU and mutex cycles on checking too frequently.
+		lastTighten time.Time
 	}
 	tighten chan struct{} // Sent on when we may want to tighten the network
 
@@ -344,6 +347,11 @@ func (s *server) gossipReceiver(
 }
 
 func (s *server) maybeTightenLocked() {
+	now := timeutil.Now()
+	if now.Before(s.mu.lastTighten.Add(gossipTightenInterval)) {
+		// It hasn't been long since we last tightened the network, so skip it.
+		return
+	}
 	select {
 	case s.tighten <- struct{}{}:
 	default:

pkg/kv/kvserver/batcheval/cmd_conditional_put.go

@@ -54,24 +54,14 @@ func ConditionalPut(
 		ts = h.Timestamp
 	}
 
-	var expVal []byte
-	if len(args.ExpBytes) != 0 {
-		expVal = args.ExpBytes
-	} else {
-		// Compatibility with 20.1 requests.
-		if args.DeprecatedExpValue != nil {
-			expVal = args.DeprecatedExpValue.TagAndDataBytes()
-		}
-	}
-
 	handleMissing := storage.CPutMissingBehavior(args.AllowIfDoesNotExist)
 	var err error
 	if args.Blind {
 		err = storage.MVCCBlindConditionalPut(
-			ctx, readWriter, cArgs.Stats, args.Key, ts, cArgs.Now, args.Value, expVal, handleMissing, h.Txn)
+			ctx, readWriter, cArgs.Stats, args.Key, ts, cArgs.Now, args.Value, args.ExpBytes, handleMissing, h.Txn)
 	} else {
 		err = storage.MVCCConditionalPut(
-			ctx, readWriter, cArgs.Stats, args.Key, ts, cArgs.Now, args.Value, expVal, handleMissing, h.Txn)
+			ctx, readWriter, cArgs.Stats, args.Key, ts, cArgs.Now, args.Value, args.ExpBytes, handleMissing, h.Txn)
 	}
 	// NB: even if MVCC returns an error, it may still have written an intent
 	// into the batch. This allows callers to consume errors like WriteTooOld

pkg/roachpb/api.go

@@ -1159,20 +1159,11 @@ func NewPutInline(key Key, value Value) Request {
 // into the request.
 func NewConditionalPut(key Key, value Value, expValue []byte, allowNotExist bool) Request {
 	value.InitChecksum(key)
-	// Compatibility with 20.1 servers.
-	var expValueVal *Value
-	if expValue != nil {
-		expValueVal = &Value{}
-		expValueVal.SetTagAndData(expValue)
-		// The expected value does not need a checksum, so we don't initialize it.
-	}
-
 	return &ConditionalPutRequest{
 		RequestHeader: RequestHeader{
 			Key: key,
 		},
 		Value:               value,
-		DeprecatedExpValue:  expValueVal,
 		ExpBytes:            expValue,
 		AllowIfDoesNotExist: allowNotExist,
 	}
@@ -1186,20 +1177,11 @@ func NewConditionalPut(key Key, value Value, expValue []byte, allowNotExist bool
 // into the request.
 func NewConditionalPutInline(key Key, value Value, expValue []byte, allowNotExist bool) Request {
 	value.InitChecksum(key)
-	// Compatibility with 20.1 servers.
-	var expValueVal *Value
-	if expValue != nil {
-		expValueVal = &Value{}
-		expValueVal.SetTagAndData(expValue)
-		// The expected value does not need a checksum, so we don't initialize it.
-	}
-
 	return &ConditionalPutRequest{
 		RequestHeader: RequestHeader{
 			Key: key,
 		},
 		Value:               value,
-		DeprecatedExpValue:  expValueVal,
 		ExpBytes:            expValue,
 		AllowIfDoesNotExist: allowNotExist,
 		Inline:              true,

pkg/roachpb/api.proto

@@ -248,15 +248,7 @@ message ConditionalPutRequest {
   RequestHeader header = 1 [(gogoproto.nullable) = false, (gogoproto.embed) = true];
   // The value to put.
   Value value = 2 [(gogoproto.nullable) = false];
-  // deprecated_exp_val represents the expected existing value for the key. If
-  // the existing value is different, the request will return a
-  // ConditionFailedError. A missing (Go nil) deprecated_exp_value.raw_bytes
-  // means that the key is expected to not exist.
-  //
-  // This is deprecated in 20.2 in favor of exp_bytes, which clarifies that the
-  // checksum and timestamp of the expected value are irrelevant. Remove in
-  // 21.1.
-  Value deprecated_exp_value = 3;
+  reserved 3;
   // exp_bytes represents the expected existing value for the key. If empty, the
   // key is expected to not exist. If not empty, these bytes are expected to
   // contain the tag and data of the existing value (without the existing