Merge #100893 #101058 #101077 #101097

100893: ui: search criteria ux improvements r=maryliag a=maryliag

Some of the names of sort on search criteria were
not a match for the column name on the tables, which could cause confusion. This commit updates the values of "P99" to "P99 Latency" and "Service Latency" to "Statement time" and "Transaction time".

Epic: None

Release note (ui change): Update sort label
on Search Criteria to match the name on the table columns.

101058: roachtest: bump tpccbench timeout r=srosenberg a=renatolabs

Looking at the test history, we see that tpccbench may sometimes take longer than 5h, especially in multi-region setups. For that reason, we bump the timeout for this test to 7h, which should be sufficient and avoid failures due to timeouts.

This commit also removes an unused `MinVersion` field in the `tpccBenchSpec` struct.

Resolves #100975.

Release note: None

101077: changefeedccl: Fix TestChangefeedHandlesDrainingNodes test r=miretskiy a=miretskiy

The test becamse flaky after #100476 merged
Fixes #100903

Release note: None

101097: server: fix a race condition during server initialization r=irfansharif a=knz

Fixes #91414.
Fixes  #101010.
Fixes #100902.

The call to `registerEnginesForDiskStatsMap` needs to wait until the store IDs are known.

Release note: None
Epic: None

Co-authored-by: maryliag <[email protected]>
Co-authored-by: Renato Costa <[email protected]>
Co-authored-by: Yevgeniy Miretskiy <[email protected]>
Co-authored-by: Raphael 'kena' Poss <[email protected]>

pkg/ccl/changefeedccl/changefeed_test.go

@@ -5972,7 +5972,7 @@ func TestChangefeedHandlesDrainingNodes(t *testing.T) {
 	sqlDB.Exec(t, "ALTER TABLE test.foo SCATTER")
 
 	// Create a factory which executes the CREATE CHANGEFEED statement on server 0.
-	// This statement should fail, but the job itself ought to be creaated.
+	// This statement should fail, but the job itself ought to be created.
 	// After some time, that job should be adopted by another node, and executed successfully.
 	f, closeSink := makeFeedFactory(t, randomSinkType(feedTestEnterpriseSinks), tc.Server(1), tc.ServerConn(0))
 	defer closeSink()
@@ -5981,22 +5981,21 @@ func TestChangefeedHandlesDrainingNodes(t *testing.T) {
 	feed := feed(t, f, "CREATE CHANGEFEED FOR foo")
 	defer closeFeed(t, feed)
 
-	// At this point, the job created by feed will fail to start running on node 0 due to draining
-	// registry.  However, this job will be retried, and it should succeed.
-	// Note: This test is a bit unrealistic in that if the registry is draining, that
-	// means that the server is draining (i.e. being shut down).  We don't do a full shutdown
-	// here, but we are simulating a restart by failing to start a flow the first time around.
-	assertPayloads(t, feed, []string{
-		`foo: [1]->{"after": {"k": 1, "v": 1}}`,
-		`foo: [2]->{"after": {"k": 2, "v": 0}}`,
-		`foo: [3]->{"after": {"k": 3, "v": 1}}`,
-		`foo: [4]->{"after": {"k": 4, "v": 0}}`,
-		`foo: [5]->{"after": {"k": 5, "v": 1}}`,
-		`foo: [6]->{"after": {"k": 6, "v": 0}}`,
-		`foo: [7]->{"after": {"k": 7, "v": 1}}`,
-		`foo: [8]->{"after": {"k": 8, "v": 0}}`,
-		`foo: [9]->{"after": {"k": 9, "v": 1}}`,
-		`foo: [10]->{"after": {"k": 10, "v": 0}}`,
+	jobID := feed.(cdctest.EnterpriseTestFeed).JobID()
+	registry := tc.Server(1).JobRegistry().(*jobs.Registry)
+	loadProgress := func() jobspb.Progress {
+		job, err := registry.LoadJob(context.Background(), jobID)
+		require.NoError(t, err)
+		return job.Progress()
+	}
+
+	// Wait until highwater advances.
+	testutils.SucceedsSoon(t, func() error {
+		progress := loadProgress()
+		if hw := progress.GetHighWater(); hw == nil || hw.IsEmpty() {
+			return errors.New("waiting for highwater")
+		}
+		return nil
 	})
 }
 

pkg/cmd/roachtest/tests/tpcc.go

@@ -979,8 +979,6 @@ type tpccBenchSpec struct {
 	// change (i.e. CockroachDB gets faster!).
 	EstimatedMax int
 
-	// MinVersion to pass to testRegistryImpl.Add.
-	MinVersion string
 	// Tags to pass to testRegistryImpl.Add.
 	Tags map[string]struct{}
 	// EncryptionEnabled determines if the benchmark uses encrypted stores (i.e.
@@ -1068,16 +1066,11 @@ func registerTPCCBenchSpec(r registry.Registry, b tpccBenchSpec) {
 	numNodes := b.Nodes + b.LoadConfig.numLoadNodes(b.Distribution)
 	nodes := r.MakeClusterSpec(numNodes, opts...)
 
-	minVersion := b.MinVersion
-	if minVersion == "" {
-		minVersion = "v19.1.0" // needed for import
-	}
-
 	r.Add(registry.TestSpec{
 		Name:              name,
 		Owner:             owner,
 		Cluster:           nodes,
-		Timeout:           5 * time.Hour,
+		Timeout:           7 * time.Hour,
 		Tags:              b.Tags,
 		EncryptionSupport: encryptionSupport,
 		Run: func(ctx context.Context, t test.Test, c cluster.Cluster) {

pkg/server/server.go

@@ -1798,6 +1798,13 @@ func (s *Server) PreStart(ctx context.Context) error {
 	// to bypass admission control.
 	s.storeGrantCoords.SetPebbleMetricsProvider(ctx, s.node, s.node)
 
+	// Connect the engines to the disk stats map constructor.
+	// This also needs to wait until after `waitForAdditionalStoreInit` returns,
+	// as the store IDs may not be known until then.
+	if err := s.node.registerEnginesForDiskStatsMap(s.cfg.Stores.Specs, s.engines); err != nil {
+		return errors.Wrapf(err, "failed to register engines for the disk stats map")
+	}
+
 	// Once all stores are initialized, check if offline storage recovery
 	// was done prior to start and record any actions appropriately.
 	logPendingLossOfQuorumRecoveryEvents(workersCtx, s.node.stores)
@@ -1938,11 +1945,6 @@ func (s *Server) PreStart(ctx context.Context) error {
 		}
 	}
 
-	// Connect the engines to the disk stats map constructor.
-	if err := s.node.registerEnginesForDiskStatsMap(s.cfg.Stores.Specs, s.engines); err != nil {
-		return errors.Wrapf(err, "failed to register engines for the disk stats map")
-	}
-
 	if storage.WorkloadCollectorEnabled {
 		if err := s.debug.RegisterWorkloadCollector(s.node.stores); err != nil {
 			return errors.Wrapf(err, "failed to register workload collector with debug server")

pkg/ui/workspaces/cluster-ui/src/statementsPage/statementsPage.tsx

@@ -308,7 +308,7 @@ export class StatementsPage extends React.Component<
       this.props.onApplySearchCriteria(
         this.state.timeScale,
         this.state.limit,
-        getSortLabel(this.state.reqSortSetting),
+        getSortLabel(this.state.reqSortSetting, "Statement"),
       );
     }
     this.refreshStatements();
@@ -594,7 +594,10 @@ export class StatementsPage extends React.Component<
     );
 
     const period = timeScaleToString(this.props.timeScale);
-    const sortSettingLabel = getSortLabel(this.props.reqSortSetting);
+    const sortSettingLabel = getSortLabel(
+      this.props.reqSortSetting,
+      "Statement",
+    );
 
     return (
       <>

pkg/ui/workspaces/cluster-ui/src/transactionsPage/transactionsPage.tsx

@@ -417,7 +417,7 @@ export class TransactionsPage extends React.Component<
       this.props.onApplySearchCriteria(
         this.state.timeScale,
         this.state.limit,
-        getSortLabel(this.state.reqSortSetting),
+        getSortLabel(this.state.reqSortSetting, "Transaction"),
       );
     }
     this.refreshData();
@@ -528,7 +528,10 @@ export class TransactionsPage extends React.Component<
     );
 
     const period = timeScaleToString(this.props.timeScale);
-    const sortSettingLabel = getSortLabel(this.props.reqSortSetting);
+    const sortSettingLabel = getSortLabel(
+      this.props.reqSortSetting,
+      "Transaction",
+    );
 
     return (
       <>

pkg/ui/workspaces/cluster-ui/src/util/sqlActivityConstants.ts

@@ -22,20 +22,23 @@ export const limitOptions = [
   { value: 500, label: "500" },
 ];
 
-export function getSortLabel(sort: SqlStatsSortType): string {
+export function getSortLabel(
+  sort: SqlStatsSortType,
+  type: "Statement" | "Transaction",
+): string {
   switch (sort) {
     case SqlStatsSortOptions.SERVICE_LAT:
-      return "Service Latency";
+      return `${type} Time`;
     case SqlStatsSortOptions.EXECUTION_COUNT:
       return "Execution Count";
     case SqlStatsSortOptions.CPU_TIME:
       return "CPU Time";
     case SqlStatsSortOptions.P99_STMTS_ONLY:
-      return "P99";
+      return "P99 Latency";
     case SqlStatsSortOptions.CONTENTION_TIME:
       return "Contention Time";
     case SqlStatsSortOptions.PCT_RUNTIME:
-      return "% Of All Runtime";
+      return "% of All Runtime";
     default:
       return "";
   }
@@ -63,19 +66,29 @@ export function getSortColumn(sort: SqlStatsSortType): string {
 export const stmtRequestSortOptions = Object.values(SqlStatsSortOptions)
   .map(sortVal => ({
     value: sortVal as SqlStatsSortType,
-    label: getSortLabel(sortVal as SqlStatsSortType),
+    label: getSortLabel(sortVal as SqlStatsSortType, "Statement"),
   }))
   .sort((a, b) => {
     if (a.label < b.label) return -1;
     if (a.label > b.label) return 1;
     return 0;
   });
 
-export const txnRequestSortOptions = stmtRequestSortOptions.filter(
-  option =>
-    option.value !== SqlStatsSortOptions.P99_STMTS_ONLY &&
-    option.value !== SqlStatsSortOptions.PCT_RUNTIME,
-);
+export const txnRequestSortOptions = Object.values(SqlStatsSortOptions)
+  .map(sortVal => ({
+    value: sortVal as SqlStatsSortType,
+    label: getSortLabel(sortVal as SqlStatsSortType, "Transaction"),
+  }))
+  .sort((a, b) => {
+    if (a.label < b.label) return -1;
+    if (a.label > b.label) return 1;
+    return 0;
+  })
+  .filter(
+    option =>
+      option.value !== SqlStatsSortOptions.P99_STMTS_ONLY &&
+      option.value !== SqlStatsSortOptions.PCT_RUNTIME,
+  );
 
 export const STATS_LONG_LOADING_DURATION = duration(2, "s");