Merge #75632 #75662 #75666 #75669

75632: execstats: fix flakey TestTraceAnalyzer r=yuzefovich a=adityamaru Fixes: #75546 Release note: None 75662: githooks: avoid accidental branch push to origin r=dt a=dt Branches are expected to be pushed to forks instead. Release note: none. 75666: kvstreamer: fix the tracing of async requests r=yuzefovich a=yuzefovich Previously, we forgot to specify a span option when running asynchronous requests and ended up using the default (which is `FollowsFromSpan` meaning that the span won't get included into the parent). The correct option to use is `ChildSpan` so that the DistSender tracing is included into the Streamer traces which is what this commit does. Release note: None 75669: bazel: bump size of `physicalplan` test r=rail a=rickystewart Release note: None Co-authored-by: Aditya Maru <[email protected]> Co-authored-by: David Taylor <[email protected]> Co-authored-by: Yahor Yuzefovich <[email protected]> Co-authored-by: Ricky Stewart <[email protected]>
cockroachdb · Jan 28, 2022 · bb85ddc · bb85ddc
5 parents f3abce6 + 16bf5d9 + 30de662 + e9583eb + ad53d77
commit bb85ddc
Show file tree

Hide file tree

Showing 5 changed files with 66 additions and 12 deletions.
diff --git a/githooks/pre-push b/githooks/pre-push
@@ -0,0 +1,20 @@
+#!/usr/bin/env bash
+#
+# -u: we want the variables to be properly assigned.
+# -o pipefail: we want to test the result of pipes.
+# No -e because we have failing commands and that's OK.
+set -uo pipefail
+
+# deny push of a head but not a tag to cockroachdb/cochroach ssh and http URLs.
+while read local_ref local_sha remote_ref remote_sha
+do 
+  if [[ "$remote_ref" == "refs/heads/"* ]] && [[ "$2" == *"cockroachdb/cockroach.git"* ]]; then
+    echo "Refusing to push to $remote_ref on $2."
+    echo "Push your branch to your own fork and open a PR from there."
+    echo "If you just want to see what CI thinks, you can push branch:refs/ci/branch to trigger a CI run."
+    echo "If this is an emergency or unusual circumstance that requires a branch on origin, push with --no-verify."
+    exit 1
+  fi
+done
+
+exit 0
diff --git a/pkg/kv/kvclient/kvstreamer/streamer.go b/pkg/kv/kvclient/kvstreamer/streamer.go
@@ -400,7 +400,14 @@ func (s *Streamer) Enqueue(
 		var coordinatorCtx context.Context
 		coordinatorCtx, s.coordinatorCtxCancel = context.WithCancel(ctx)
 		s.waitGroup.Add(1)
-		if err := s.stopper.RunAsyncTask(coordinatorCtx, "streamer-coordinator", s.coordinator.mainLoop); err != nil {
+		if err := s.stopper.RunAsyncTaskEx(
+			coordinatorCtx,
+			stop.TaskOpts{
+				TaskName: "streamer-coordinator",
+				SpanOpt:  stop.ChildSpan,
+			},
+			s.coordinator.mainLoop,
+		); err != nil {
 			// The new goroutine wasn't spun up, so mainLoop won't get executed
 			// and we have to decrement the wait group ourselves.
 			s.waitGroup.Done()
@@ -999,6 +1006,7 @@ func (w *workerCoordinator) performRequestAsync(
 		ctx,
 		stop.TaskOpts{
 			TaskName:   "streamer-lookup-async",
+			SpanOpt:    stop.ChildSpan,
 			Sem:        w.asyncSem,
 			WaitForSem: true,
 		},

diff --git a/pkg/sql/execstats/traceanalyzer_test.go b/pkg/sql/execstats/traceanalyzer_test.go
@@ -107,17 +107,14 @@ func TestTraceAnalyzer(t *testing.T) {
 	for _, vectorizeMode := range []sessiondatapb.VectorizeExecMode{sessiondatapb.VectorizeOff, sessiondatapb.VectorizeOn} {
 		execCtx, finishAndCollect := tracing.ContextWithRecordingSpan(ctx, execCfg.AmbientCtx.Tracer, t.Name())
 		defer finishAndCollect()
-		ie := execCfg.InternalExecutor
-		ie.SetSessionData(
-			&sessiondata.SessionData{
-				SessionData: sessiondatapb.SessionData{
-					VectorizeMode: vectorizeMode,
-				},
-				LocalOnlySessionData: sessiondatapb.LocalOnlySessionData{
-					DistSQLMode: sessiondatapb.DistSQLOn,
-				},
+		ie := execCfg.InternalExecutorFactory(ctx, &sessiondata.SessionData{
+			SessionData: sessiondatapb.SessionData{
+				VectorizeMode: vectorizeMode,
 			},
-		)
+			LocalOnlySessionData: sessiondatapb.LocalOnlySessionData{
+				DistSQLMode: sessiondatapb.DistSQLOn,
+			},
+		})
 		_, err := ie.ExecEx(
 			execCtx,
 			t.Name(),

diff --git a/pkg/sql/opt/exec/execbuilder/testdata/show_trace_nonmetamorphic b/pkg/sql/opt/exec/execbuilder/testdata/show_trace_nonmetamorphic
@@ -374,3 +374,32 @@ dist sender send  r44: sending batch 6 CPut to (n1,s1):1
 dist sender send  r44: sending batch 6 CPut to (n1,s1):1
 dist sender send  r44: sending batch 6 CPut to (n1,s1):1
 dist sender send  r44: sending batch 1 EndTxn to (n1,s1):1
+
+statement ok
+CREATE TABLE streamer (pk INT PRIMARY KEY, attribute INT, blob TEXT, INDEX(attribute), FAMILY (pk, attribute, blob));
+INSERT INTO streamer SELECT i, 1, repeat('a', 10) FROM generate_series(1, 42) AS g(i);
+
+# Get the range id.
+let $rangeid
+SELECT range_id FROM [ SHOW RANGES FROM TABLE streamer ]
+
+# Populate table descriptor cache.
+statement ok
+SELECT * FROM streamer
+
+# Trace the statement that performs a point read followed by an index join that
+# is implemented via the Streamer API.
+statement ok
+SET tracing = on;
+SELECT * FROM streamer@streamer_attribute_idx WHERE attribute=1;
+SET tracing = off;
+
+# The index join will issue a batch with 42 Get requests, so we want to verify
+# that the corresponding log is included into the trace by the DistSender.
+query TT
+SELECT operation, message FROM [SHOW KV TRACE FOR SESSION]
+WHERE message     LIKE '%r$rangeid: sending batch 42 Get%'
+  AND message NOT LIKE '%PushTxn%'
+  AND message NOT LIKE '%QueryTxn%'
+----
+dist sender send  r44: sending batch 42 Get to (n1,s1):1
diff --git a/pkg/sql/physicalplan/BUILD.bazel b/pkg/sql/physicalplan/BUILD.bazel
@@ -35,7 +35,7 @@ go_library(
 
 go_test(
     name = "physicalplan_test",
-    size = "medium",
+    size = "large",
     srcs = [
         "aggregator_funcs_test.go",
         "fake_span_resolver_test.go",