From c485cc5c7477315408173dcf3f1f5cbe0456f7c8 Mon Sep 17 00:00:00 2001 From: irfan sharif Date: Thu, 28 Oct 2021 18:50:29 -0400 Subject: [PATCH 1/4] dev: pad internal timeouts when under --stress --timeout I was seeing the earlier one second buffer being insufficient. Release note: None --- pkg/cmd/dev/test.go | 6 +++--- pkg/cmd/dev/testdata/recording/test.txt | 2 +- pkg/cmd/dev/testdata/test.txt | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/pkg/cmd/dev/test.go b/pkg/cmd/dev/test.go index 283d8b34093d..738608203ce3 100644 --- a/pkg/cmd/dev/test.go +++ b/pkg/cmd/dev/test.go @@ -180,9 +180,9 @@ func (d *dev) test(cmd *cobra.Command, commandLine []string) error { args = append(args, "--run_under", fmt.Sprintf("%s -maxtime=%s %s", stressTarget, timeout, stressArgs)) - // The bazel timeout needs to be higher than the stress duration to - // pass reliably. - args = append(args, fmt.Sprintf("--test_timeout=%.0f", (timeout+time.Second).Seconds())) + // The timeout should be higher than the stress duration, lets + // generously give it an extra minute. + args = append(args, fmt.Sprintf("--test_timeout=%d", int((timeout+time.Minute).Seconds()))) } else { // We're running under stress and no timeout is specified. We want // to respect the timeout passed down to stress[1]. Similar to above diff --git a/pkg/cmd/dev/testdata/recording/test.txt b/pkg/cmd/dev/testdata/recording/test.txt index 6f438e02163a..51ea3d0f69ab 100644 --- a/pkg/cmd/dev/testdata/recording/test.txt +++ b/pkg/cmd/dev/testdata/recording/test.txt @@ -97,7 +97,7 @@ bazel query 'kind(go_test, //pkg/util/tracing:all)' ---- //pkg/util/tracing:tracing_test -bazel test --test_sharding_strategy=disabled //pkg/util/tracing:tracing_test --run_under '@com_github_cockroachdb_stress//:stress -maxtime=10s ' --test_timeout=11 '--test_filter=TestStartChild*' --test_output streamed +bazel test --test_sharding_strategy=disabled //pkg/util/tracing:tracing_test --run_under '@com_github_cockroachdb_stress//:stress -maxtime=10s ' --test_timeout=70 '--test_filter=TestStartChild*' --test_output streamed ---- ---- ==================== Test output for //pkg/util/tracing:tracing_test: diff --git a/pkg/cmd/dev/testdata/test.txt b/pkg/cmd/dev/testdata/test.txt index 492431d6ace6..1fb3685efab0 100644 --- a/pkg/cmd/dev/testdata/test.txt +++ b/pkg/cmd/dev/testdata/test.txt @@ -36,7 +36,7 @@ bazel test --test_sharding_strategy=disabled //pkg/util/tracing:tracing_test --r dev test --stress pkg/util/tracing --filter TestStartChild* --timeout=10s -v ---- bazel query 'kind(go_test, //pkg/util/tracing:all)' -bazel test --test_sharding_strategy=disabled //pkg/util/tracing:tracing_test --run_under '@com_github_cockroachdb_stress//:stress -maxtime=10s ' --test_timeout=11 '--test_filter=TestStartChild*' --test_output streamed +bazel test --test_sharding_strategy=disabled //pkg/util/tracing:tracing_test --run_under '@com_github_cockroachdb_stress//:stress -maxtime=10s ' --test_timeout=70 '--test_filter=TestStartChild*' --test_output streamed dev test //pkg/testutils --timeout=10s ---- From b08c31d5c34c35ae34e048b55f49b5af29904474 Mon Sep 17 00:00:00 2001 From: Yevgeniy Miretskiy Date: Sat, 30 Oct 2021 08:27:40 -0400 Subject: [PATCH 2/4] changefeedccl: Move metrics scope library under changefeedccl package. Move metrics scope library out of cdcutil directly under changefeedccl. Metrics scope is not much of a library since it's specific to changefeedccl. Note: this is a move only refactoring, but it is structured as 2 changes (one to add new files, and one to remove cdcutils files) so that it can be easily backported. Release Notes: None --- pkg/ccl/changefeedccl/BUILD.bazel | 3 + pkg/ccl/changefeedccl/metrics_scope.go | 77 +++++++++++++++++++++ pkg/ccl/changefeedccl/metrics_scope_test.go | 57 +++++++++++++++ 3 files changed, 137 insertions(+) create mode 100644 pkg/ccl/changefeedccl/metrics_scope.go create mode 100644 pkg/ccl/changefeedccl/metrics_scope_test.go diff --git a/pkg/ccl/changefeedccl/BUILD.bazel b/pkg/ccl/changefeedccl/BUILD.bazel index 14bbc91e56f3..ad6d298d400f 100644 --- a/pkg/ccl/changefeedccl/BUILD.bazel +++ b/pkg/ccl/changefeedccl/BUILD.bazel @@ -11,6 +11,7 @@ go_library( "doc.go", "encoder.go", "metrics.go", + "metrics_scope.go", "name.go", "rowfetcher_cache.go", "schema_registry.go", @@ -122,6 +123,7 @@ go_test( "helpers_tenant_shim_test.go", "helpers_test.go", "main_test.go", + "metrics_scope_test.go", "name_test.go", "nemeses_test.go", "schema_registry_test.go", @@ -200,6 +202,7 @@ go_test( "//pkg/util/json", "//pkg/util/leaktest", "//pkg/util/log", + "//pkg/util/metric", "//pkg/util/mon", "//pkg/util/protoutil", "//pkg/util/randutil", diff --git a/pkg/ccl/changefeedccl/metrics_scope.go b/pkg/ccl/changefeedccl/metrics_scope.go new file mode 100644 index 000000000000..3feeaf2db71a --- /dev/null +++ b/pkg/ccl/changefeedccl/metrics_scope.go @@ -0,0 +1,77 @@ +// Copyright 2021 The Cockroach Authors. +// +// Licensed as a CockroachDB Enterprise file under the Cockroach Community +// License (the "License"); you may not use this file except in compliance with +// the License. You may obtain a copy of the License at +// +// https://github.com/cockroachdb/cockroach/blob/master/licenses/CCL.txt + +package changefeedccl + +import ( + "fmt" + + "github.com/cockroachdb/cockroach/pkg/util/metric" +) + +// maxSLIScopes is a static limit on the number of SLI scopes -- that is the number +// of SLI metrics we will keep track of. +// The limit is static due to metric.Registry limitations. +const maxSLIScopes = 8 + +// SLIMetrics is the list of SLI related metrics for changefeeds. +type SLIMetrics struct { + ErrorRetries *metric.Counter + // TODO(yevgeniy): Add more SLI related metrics. +} + +// MetricStruct implements metric.Struct interface +func (*SLIMetrics) MetricStruct() {} + +func makeSLIMetrics(prefix string) *SLIMetrics { + withPrefix := func(meta metric.Metadata) metric.Metadata { + meta.Name = fmt.Sprintf("%s.%s", prefix, meta.Name) + return meta + } + + return &SLIMetrics{ + ErrorRetries: metric.NewCounter(withPrefix(metric.Metadata{ + Name: "error_retries", + Help: "Total retryable errors encountered this SLI", + Measurement: "Errors", + Unit: metric.Unit_COUNT, + })), + } +} + +// SLIScopes represents a set of SLI related metrics for a particular "scope". +type SLIScopes struct { + Scopes [maxSLIScopes]*SLIMetrics // Exported so that we can register w/ metrics registry. + names map[string]*SLIMetrics +} + +// MetricStruct implements metric.Struct interface +func (*SLIScopes) MetricStruct() {} + +// CreateSLIScopes creates changefeed specific SLI scope: a metric.Struct containing +// SLI specific metrics for each scope. +// The scopes are statically named "tier", and each metric name +// contained in SLIMetrics will be prefixed by "changefeed.tier Date: Sat, 30 Oct 2021 08:31:47 -0400 Subject: [PATCH 3/4] changefeedccl: Remove metrics scope library. Remove metrics scope library since it has been moved under changefeedccl. Note: this is a move only refactoring, but it is structured as 2 changes (one to add new files, and one to remove cdcutils files) so that it can be easily backported. Release Notes: None --- pkg/ccl/changefeedccl/cdcutils/BUILD.bazel | 11 +-- .../changefeedccl/cdcutils/metrics_scope.go | 77 ------------------- .../cdcutils/metrics_scope_test.go | 57 -------------- 3 files changed, 2 insertions(+), 143 deletions(-) delete mode 100644 pkg/ccl/changefeedccl/cdcutils/metrics_scope.go delete mode 100644 pkg/ccl/changefeedccl/cdcutils/metrics_scope_test.go diff --git a/pkg/ccl/changefeedccl/cdcutils/BUILD.bazel b/pkg/ccl/changefeedccl/cdcutils/BUILD.bazel index a9dfffd76a83..993c43479b4e 100644 --- a/pkg/ccl/changefeedccl/cdcutils/BUILD.bazel +++ b/pkg/ccl/changefeedccl/cdcutils/BUILD.bazel @@ -2,10 +2,7 @@ load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test") go_library( name = "cdcutils", - srcs = [ - "metrics_scope.go", - "throttle.go", - ], + srcs = ["throttle.go"], importpath = "github.com/cockroachdb/cockroach/pkg/ccl/changefeedccl/cdcutils", visibility = ["//visibility:public"], deps = [ @@ -21,17 +18,13 @@ go_library( go_test( name = "cdcutils_test", - srcs = [ - "metrics_scope_test.go", - "throttle_test.go", - ], + srcs = ["throttle_test.go"], embed = [":cdcutils"], deps = [ "//pkg/ccl/changefeedccl/changefeedbase", "//pkg/settings/cluster", "//pkg/util/leaktest", "//pkg/util/log", - "//pkg/util/metric", "@com_github_stretchr_testify//require", ], ) diff --git a/pkg/ccl/changefeedccl/cdcutils/metrics_scope.go b/pkg/ccl/changefeedccl/cdcutils/metrics_scope.go deleted file mode 100644 index a69430dbe54a..000000000000 --- a/pkg/ccl/changefeedccl/cdcutils/metrics_scope.go +++ /dev/null @@ -1,77 +0,0 @@ -// Copyright 2021 The Cockroach Authors. -// -// Licensed as a CockroachDB Enterprise file under the Cockroach Community -// License (the "License"); you may not use this file except in compliance with -// the License. You may obtain a copy of the License at -// -// https://github.com/cockroachdb/cockroach/blob/master/licenses/CCL.txt - -package cdcutils - -import ( - "fmt" - - "github.com/cockroachdb/cockroach/pkg/util/metric" -) - -// maxSLIScopes is a static limit on the number of SLI scopes -- that is the number -// of SLI metrics we will keep track of. -// The limit is static due to metric.Registry limitations. -const maxSLIScopes = 8 - -// SLIMetrics is the list of SLI related metrics for changefeeds. -type SLIMetrics struct { - ErrorRetries *metric.Counter - // TODO(yevgeniy): Add more SLI related metrics. -} - -// MetricStruct implements metric.Struct interface -func (*SLIMetrics) MetricStruct() {} - -func makeSLIMetrics(prefix string) *SLIMetrics { - withPrefix := func(meta metric.Metadata) metric.Metadata { - meta.Name = fmt.Sprintf("%s.%s", prefix, meta.Name) - return meta - } - - return &SLIMetrics{ - ErrorRetries: metric.NewCounter(withPrefix(metric.Metadata{ - Name: "error_retries", - Help: "Total retryable errors encountered this SLI", - Measurement: "Errors", - Unit: metric.Unit_COUNT, - })), - } -} - -// SLIScopes represents a set of SLI related metrics for a particular "scope". -type SLIScopes struct { - Scopes [maxSLIScopes]*SLIMetrics // Exported so that we can register w/ metrics registry. - names map[string]*SLIMetrics -} - -// MetricStruct implements metric.Struct interface -func (*SLIScopes) MetricStruct() {} - -// CreateSLIScopes creates changefeed specific SLI scope: a metric.Struct containing -// SLI specific metrics for each scope. -// The scopes are statically named "tier", and each metric name -// contained in SLIMetrics will be prefixed by "changefeed.tier Date: Thu, 28 Oct 2021 10:40:45 -0500 Subject: [PATCH 4/4] bazel: glob-exclude all `*_generated.go` files from Gazelle This will help ensure that Gazelle doesn't accidentally reference checked-in generated code. The generated code in `pkg/col/colserde/arrowserde` is made by `flatc`, the FlatBuffers compiler -- I tried to make a `genrule` to generate this code but the code that `flatc` generates now doesn't match up to what's checked in (maybe we used a different version of `flatc` for the original version of the code?) Closes #72096. Release note: None --- BUILD.bazel | 19 +------------------ pkg/col/colserde/arrowserde/BUILD.bazel | 12 +++++++----- 2 files changed, 8 insertions(+), 23 deletions(-) diff --git a/BUILD.bazel b/BUILD.bazel index 18fe230a77e3..da1edd0c12c9 100644 --- a/BUILD.bazel +++ b/BUILD.bazel @@ -69,7 +69,6 @@ load("@bazel_gazelle//:def.bzl", "gazelle") # gazelle:resolve proto go roachpb/io-formats.proto //pkg/roachpb:with-mocks # gazelle:resolve proto go roachpb/metadata.proto //pkg/roachpb:with-mocks # gazelle:resolve proto go roachpb/span_config.proto //pkg/roachpb:with-mocks -# gazelle:exclude pkg/roachpb/batch_generated.go # gazelle:exclude pkg/roachpb/batch_generated-gen.go # See pkg/sql/opt/optgen/cmd/langgen/BUILD.bazel for more details. @@ -107,7 +106,7 @@ load("@bazel_gazelle//:def.bzl", "gazelle") # gazelle:exclude **/*.pb.gw.go # gazelle:exclude **/*_interval_btree.go # gazelle:exclude **/*_interval_btree_test.go -# gazelle:exclude **/mocks_generated.go +# gazelle:exclude **/*_generated.go # gazelle:exclude pkg/sql/parser/sql.go # gazelle:exclude pkg/sql/parser/helpmap_test.go # gazelle:exclude pkg/sql/parser/help_messages.go @@ -118,25 +117,9 @@ load("@bazel_gazelle//:def.bzl", "gazelle") # gazelle:exclude pkg/testutils/**/testdata/** # gazelle:exclude pkg/security/securitytest/embedded.go # gazelle:exclude pkg/cmd/roachprod/vm/aws/embedded.go -# gazelle:exclude pkg/cmd/roachtest/prometheus/mock_generated.go -# gazelle:exclude pkg/cmd/roachtest/tests/drt_generated.go # gazelle:exclude pkg/**/*_string.go -# gazelle:exclude pkg/geo/wkt/wkt_generated.go -# gazelle:exclude pkg/sql/schemachanger/scop/backfill_visitor_generated.go -# gazelle:exclude pkg/sql/schemachanger/scop/mutation_visitor_generated.go -# gazelle:exclude pkg/sql/schemachanger/scop/validation_visitor_generated.go -# gazelle:exclude pkg/sql/schemachanger/scpb/elements_generated.go # gazelle:exclude pkg/ui/distccl/distccl_no_bazel.go # gazelle:exclude pkg/ui/distoss/distoss_no_bazel.go -# gazelle:exclude pkg/util/log/channel/channel_generated.go -# gazelle:exclude pkg/util/log/eventpb/eventlog_channels_generated.go -# gazelle:exclude pkg/util/log/eventpb/json_encode_generated.go -# gazelle:exclude pkg/util/log/log_channels_generated.go -# gazelle:exclude pkg/util/log/severity/severity_generated.go -# gazelle:exclude pkg/util/timeutil/lowercase_timezones_generated.go -# -# TODO(irfansharif): Hand excluding these _generated.go/_stringer.go files is -# silly, we should glob exclude everything once we have full coverage. # # Generally useful references: # diff --git a/pkg/col/colserde/arrowserde/BUILD.bazel b/pkg/col/colserde/arrowserde/BUILD.bazel index c428649ff886..6d5a6ce74764 100644 --- a/pkg/col/colserde/arrowserde/BUILD.bazel +++ b/pkg/col/colserde/arrowserde/BUILD.bazel @@ -4,12 +4,14 @@ go_library( name = "arrowserde", srcs = [ "doc.go", - "file_generated.go", - "message_generated.go", - "schema_generated.go", - "tensor_generated.go", + "file_generated.go", # keep + "message_generated.go", # keep + "schema_generated.go", # keep + "tensor_generated.go", # keep ], importpath = "github.com/cockroachdb/cockroach/pkg/col/colserde/arrowserde", visibility = ["//visibility:public"], - deps = ["@com_github_google_flatbuffers//go"], + deps = [ + "@com_github_google_flatbuffers//go", # keep + ], )