Merge #96785 #97308

96785: batcheval: enforce lease proposed ts r=pavelkalinnikov a=tbg It is always set, except in some tests. Now these tests set it, too. Release note: None Epic: None 97308: copy: fix issue error message being non-deterministic in tests r=cucaroach a=otan Regexp out version numbers in issues. Resolves #97236 Release note: None Co-authored-by: Tobias Grieger <[email protected]> Co-authored-by: Oliver Tan <[email protected]>
cockroachdb · Feb 20, 2023 · 6848307 · 6848307
3 parents 744ef3c + 48fa56b + 54d9d0d
commit 6848307
Show file tree

Hide file tree

Showing 6 changed files with 24 additions and 81 deletions.
diff --git a/pkg/kv/kvserver/batcheval/cmd_lease.go b/pkg/kv/kvserver/batcheval/cmd_lease.go
@@ -22,6 +22,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
 	"github.com/cockroachdb/cockroach/pkg/storage"
 	"github.com/cockroachdb/cockroach/pkg/storage/enginepb"
+	"github.com/cockroachdb/errors"
 )
 
 func newFailedLeaseTrigger(isTransfer bool) result.Result {
@@ -62,6 +63,10 @@ func evalNewLease(
 	// When returning an error from this method, must always return
 	// a newFailedLeaseTrigger() to satisfy stats.
 
+	if lease.ProposedTS == nil {
+		return newFailedLeaseTrigger(isTransfer), errors.AssertionFailedf("ProposedTS must be set")
+	}
+
 	// Ensure either an Epoch is set or Start < Expiration.
 	if (lease.Type() == roachpb.LeaseExpiration && lease.GetExpiration().LessEq(lease.Start.ToTimestamp())) ||
 		(lease.Type() == roachpb.LeaseEpoch && lease.Expiration != nil) {

diff --git a/pkg/kv/kvserver/batcheval/cmd_lease_test.go b/pkg/kv/kvserver/batcheval/cmd_lease_test.go
@@ -201,9 +201,11 @@ func TestLeaseTransferForwardsStartTime(t *testing.T) {
 				Replica:  replicas[0],
 				Sequence: 1,
 			}
+			now := clock.NowAsClockTimestamp()
 			nextLease := roachpb.Lease{
-				Replica: replicas[1],
-				Start:   clock.NowAsClockTimestamp(),
+				ProposedTS: &now,
+				Replica:    replicas[1],
+				Start:      now,
 			}
 			if epoch {
 				nextLease.Epoch = 1

diff --git a/pkg/kv/kvserver/client_replica_test.go b/pkg/kv/kvserver/client_replica_test.go
@@ -2285,14 +2285,16 @@ func TestLeaseExtensionNotBlockedByRead(t *testing.T) {
 			t.Fatalf("replica descriptor for key %s not found", rKey)
 		}
 
+		now := s.Clock().NowAsClockTimestamp()
 		leaseReq := kvpb.RequestLeaseRequest{
 			RequestHeader: kvpb.RequestHeader{
 				Key: key,
 			},
 			Lease: roachpb.Lease{
-				Start:      s.Clock().NowAsClockTimestamp(),
-				Expiration: s.Clock().Now().Add(time.Second.Nanoseconds(), 0).Clone(),
+				Start:      now,
+				Expiration: now.ToTimestamp().Add(time.Second.Nanoseconds(), 0).Clone(),
 				Replica:    replDesc,
+				ProposedTS: &now,
 			},
 		}
 

diff --git a/pkg/kv/kvserver/replica_test.go b/pkg/kv/kvserver/replica_test.go
@@ -29,7 +29,6 @@ import (
 
 	"github.com/cockroachdb/cockroach/pkg/base"
 	"github.com/cockroachdb/cockroach/pkg/cli/exit"
-	"github.com/cockroachdb/cockroach/pkg/clusterversion"
 	"github.com/cockroachdb/cockroach/pkg/gossip"
 	"github.com/cockroachdb/cockroach/pkg/keys"
 	"github.com/cockroachdb/cockroach/pkg/kv"
@@ -49,7 +48,6 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/kv/kvserver/uncertainty"
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
 	"github.com/cockroachdb/cockroach/pkg/server/telemetry"
-	"github.com/cockroachdb/cockroach/pkg/settings/cluster"
 	"github.com/cockroachdb/cockroach/pkg/storage"
 	"github.com/cockroachdb/cockroach/pkg/storage/enginepb"
 	"github.com/cockroachdb/cockroach/pkg/testutils"
@@ -570,6 +568,7 @@ func TestReplicaReadConsistency(t *testing.T) {
 	tc.manualClock.MustAdvanceTo(leaseExpiry(tc.repl))
 	start := tc.Clock().NowAsClockTimestamp()
 	if err := sendLeaseRequest(tc.repl, &roachpb.Lease{
+		ProposedTS: &start,
 		Start:      start,
 		Expiration: start.ToTimestamp().Add(10, 0).Clone(),
 		Replica:    secondReplica,
@@ -788,6 +787,7 @@ func TestApplyCmdLeaseError(t *testing.T) {
 	tc.manualClock.MustAdvanceTo(leaseExpiry(tc.repl))
 	start := tc.Clock().NowAsClockTimestamp()
 	if err := sendLeaseRequest(tc.repl, &roachpb.Lease{
+		ProposedTS: &start,
 		Start:      start,
 		Expiration: start.ToTimestamp().Add(10, 0).Clone(),
 		Replica:    secondReplica,
@@ -944,6 +944,7 @@ func TestReplicaLease(t *testing.T) {
 	tc.manualClock.MustAdvanceTo(leaseExpiry(tc.repl))
 	now := tc.Clock().NowAsClockTimestamp()
 	if err := sendLeaseRequest(tc.repl, &roachpb.Lease{
+		ProposedTS: &now,
 		Start:      now.ToTimestamp().Add(10, 0).UnsafeToClockTimestamp(),
 		Expiration: now.ToTimestamp().Add(20, 0).Clone(),
 		Replica:    secondReplica,
@@ -999,6 +1000,7 @@ func TestReplicaNotLeaseHolderError(t *testing.T) {
 	tc.manualClock.MustAdvanceTo(leaseExpiry(tc.repl))
 	now := tc.Clock().NowAsClockTimestamp()
 	if err := sendLeaseRequest(tc.repl, &roachpb.Lease{
+		ProposedTS: &now,
 		Start:      now,
 		Expiration: now.ToTimestamp().Add(10, 0).Clone(),
 		Replica:    secondReplica,
@@ -1083,6 +1085,7 @@ func TestReplicaLeaseCounters(t *testing.T) {
 
 	now := tc.Clock().NowAsClockTimestamp()
 	if err := sendLeaseRequest(tc.repl, &roachpb.Lease{
+		ProposedTS: &now,
 		Start:      now,
 		Expiration: now.ToTimestamp().Add(10, 0).Clone(),
 		Replica: roachpb.ReplicaDescriptor{
@@ -1113,6 +1116,7 @@ func TestReplicaLeaseCounters(t *testing.T) {
 
 	// Make lease request fail by requesting overlapping lease from bogus Replica.
 	if err := sendLeaseRequest(tc.repl, &roachpb.Lease{
+		ProposedTS: &now,
 		Start:      now,
 		Expiration: now.ToTimestamp().Add(10, 0).Clone(),
 		Replica: roachpb.ReplicaDescriptor{
@@ -1136,79 +1140,6 @@ func TestReplicaLeaseCounters(t *testing.T) {
 	}
 }
 
-// TestReplicaGossipConfigsOnLease verifies that config info is gossiped
-// upon acquisition of the range lease.
-//
-// TODO(ajwerner): Delete this test in 22.2.
-func TestReplicaGossipConfigsOnLease(t *testing.T) {
-	defer leaktest.AfterTest(t)()
-	defer log.Scope(t).Close(t)
-
-	ctx := context.Background()
-	stopper := stop.NewStopper()
-	defer stopper.Stop(ctx)
-
-	tc := testContext{manualClock: timeutil.NewManualTime(timeutil.Unix(0, 123))}
-	cfg := TestStoreConfig(hlc.NewClockForTesting(tc.manualClock))
-	cfg.TestingKnobs.DisableAutomaticLeaseRenewal = true
-	// Use the TestingBinaryMinSupportedVersion for bootstrap because we won't
-	// gossip the system config once the current version is finalized.
-	cfg.Settings = cluster.MakeTestingClusterSettingsWithVersions(
-		clusterversion.TestingBinaryVersion,
-		clusterversion.TestingBinaryMinSupportedVersion,
-		false,
-	)
-	require.NoError(t, cfg.Settings.Version.SetActiveVersion(ctx, clusterversion.ClusterVersion{
-		Version: clusterversion.TestingBinaryMinSupportedVersion,
-	}))
-	tc.StartWithStoreConfigAndVersion(ctx, t, stopper, cfg,
-		clusterversion.TestingBinaryMinSupportedVersion)
-
-	secondReplica, err := tc.addBogusReplicaToRangeDesc(ctx)
-	if err != nil {
-		t.Fatal(err)
-	}
-
-	// Write some arbitrary data in the system config span.
-	key := keys.SystemSQLCodec.TablePrefix(keys.DeprecatedMaxSystemConfigDescID)
-	var val roachpb.Value
-	val.SetInt(42)
-	if err := storage.MVCCPut(context.Background(), tc.engine, nil, key, hlc.Timestamp{}, hlc.ClockTimestamp{}, val, nil); err != nil {
-		t.Fatal(err)
-	}
-
-	// Expire our own lease which we automagically acquired due to being
-	// first range and config holder.
-	tc.manualClock.MustAdvanceTo(leaseExpiry(tc.repl))
-	now := tc.Clock().NowAsClockTimestamp()
-
-	// Give lease to someone else.
-	if err := sendLeaseRequest(tc.repl, &roachpb.Lease{
-		Start:      now,
-		Expiration: now.ToTimestamp().Add(10, 0).Clone(),
-		Replica:    secondReplica,
-	}); err != nil {
-		t.Fatal(err)
-	}
-
-	// Expire that lease.
-	tc.manualClock.Advance(11 + tc.Clock().MaxOffset()) // advance time
-	now = tc.Clock().NowAsClockTimestamp()
-
-	// Give lease to this range.
-	if err := sendLeaseRequest(tc.repl, &roachpb.Lease{
-		Start:      now.ToTimestamp().Add(11, 0).UnsafeToClockTimestamp(),
-		Expiration: now.ToTimestamp().Add(20, 0).Clone(),
-		Replica: roachpb.ReplicaDescriptor{
-			ReplicaID: 1,
-			NodeID:    1,
-			StoreID:   1,
-		},
-	}); err != nil {
-		t.Fatal(err)
-	}
-}
-
 // TestReplicaTSCacheLowWaterOnLease verifies that the low water mark
 // is set on the timestamp cache when the node is granted the lease holder
 // lease after not holding it and it is not set when the node is

diff --git a/pkg/sql/copy/copy_test.go b/pkg/sql/copy/copy_test.go
@@ -18,6 +18,7 @@ import (
 	"fmt"
 	"io"
 	"net/url"
+	"regexp"
 	"runtime/pprof"
 	"strings"
 	"testing"
@@ -145,6 +146,8 @@ func TestCopy(t *testing.T) {
 	})
 }
 
+var issueLinkRE = regexp.MustCompile("https://go.crdb.dev/issue-v/([0-9]+)/.*")
+
 func expandErrorString(err error) string {
 	var sb strings.Builder
 	sb.WriteString(err.Error())
@@ -157,7 +160,7 @@ func expandErrorString(err error) string {
 			sb.WriteString(fmt.Sprintf("\nDETAIL: %s", pgErr.Detail))
 		}
 	}
-	return sb.String()
+	return issueLinkRE.ReplaceAllString(sb.String(), `https://go.crdb.dev/issue-v/$1/`)
 }
 
 // TestCopyFromTransaction tests that copy from rows are written with

diff --git a/pkg/sql/copy/testdata/copy_to_binary b/pkg/sql/copy/testdata/copy_to_binary
@@ -15,4 +15,4 @@ COPY t TO STDOUT BINARY
 ----
 ERROR: unimplemented: binary format for COPY TO not implemented (SQLSTATE 0A000)
 HINT: You have attempted to use a feature that is not yet implemented.
-See: https://go.crdb.dev/issue-v/97180/v23.1
+See: https://go.crdb.dev/issue-v/97180/