From 16bf5d96b2b77dab0e72bb87b9db632219ab95e3 Mon Sep 17 00:00:00 2001 From: Aditya Maru Date: Fri, 28 Jan 2022 09:23:23 -0500 Subject: [PATCH 1/5] execstats: fix flakey TestTraceAnalyzer With the spanconfig job running in the background, and parts of it using the internal executor, it is incorrect for this test to reach into SetSessionData. This method has a comment indicating that it is not to be used concurrently with query execution. The fix is to create a session bound internal executor in the test. Fixes: #75546 Release note: None --- pkg/sql/execstats/traceanalyzer_test.go | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/pkg/sql/execstats/traceanalyzer_test.go b/pkg/sql/execstats/traceanalyzer_test.go index 948f053922ae..299654f11d63 100644 --- 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(), From a6d2426175a36fe2b7f96ae206cd80b61c67d50b Mon Sep 17 00:00:00 2001 From: David Taylor Date: Fri, 28 Jan 2022 18:40:14 +0000 Subject: [PATCH 2/5] githooks: avoid accidental branch push to origin Branches are expected to be pushed to forks instead. Release note: none. --- githooks/pre-push | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) create mode 100755 githooks/pre-push diff --git a/githooks/pre-push b/githooks/pre-push new file mode 100755 index 000000000000..0a7ce922c21e --- /dev/null +++ b/githooks/pre-push @@ -0,0 +1,19 @@ +#!/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 this is an emergency and you need to skip this check, use --no-verify)." + exit 1 + fi +done + +exit 0 From e9583eb17e20d223aa2720024d61da5366bc909c Mon Sep 17 00:00:00 2001 From: Yahor Yuzefovich Date: Fri, 28 Jan 2022 11:08:02 -0800 Subject: [PATCH 3/5] kvstreamer: fix the tracing of async requests 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 --- pkg/kv/kvclient/kvstreamer/streamer.go | 10 ++++++- .../testdata/show_trace_nonmetamorphic | 29 +++++++++++++++++++ 2 files changed, 38 insertions(+), 1 deletion(-) diff --git a/pkg/kv/kvclient/kvstreamer/streamer.go b/pkg/kv/kvclient/kvstreamer/streamer.go index 63a13e6dd8df..7958a6946385 100644 --- 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/opt/exec/execbuilder/testdata/show_trace_nonmetamorphic b/pkg/sql/opt/exec/execbuilder/testdata/show_trace_nonmetamorphic index f2328dcefaa2..e1c5569f2455 100644 --- 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 From ad53d7703dd4252b87db362efa44e545ce2c1a87 Mon Sep 17 00:00:00 2001 From: Ricky Stewart Date: Fri, 28 Jan 2022 14:32:36 -0600 Subject: [PATCH 4/5] bazel: bump size of `physicalplan` test Release note: None --- pkg/sql/physicalplan/BUILD.bazel | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/sql/physicalplan/BUILD.bazel b/pkg/sql/physicalplan/BUILD.bazel index 4a0842818be2..01a810048c43 100644 --- 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", From 30de662f48964ab074faae03aa9855c5e8982192 Mon Sep 17 00:00:00 2001 From: David Taylor Date: Fri, 28 Jan 2022 21:31:23 +0000 Subject: [PATCH 5/5] githooks: mention refs/ci prefix in pre-push refusal Release note: none. --- githooks/pre-push | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/githooks/pre-push b/githooks/pre-push index 0a7ce922c21e..36416dc550be 100755 --- a/githooks/pre-push +++ b/githooks/pre-push @@ -11,7 +11,8 @@ 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 this is an emergency and you need to skip this check, use --no-verify)." + 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