From c57a8d9ff1df488b9ee296fa8b33b490aaab9f3c Mon Sep 17 00:00:00 2001 From: Yuxuan 'fishy' Wang Date: Fri, 8 Sep 2023 12:12:18 -0700 Subject: [PATCH] Update go support to 1.20+1.21 Also upgrade thrift to 0.19.0. Also deprecate errorsbp.Batch. --- .github/workflows/go-ci.yaml | 2 +- batchcloser/closers.go | 7 +-- errorsbp/batch.go | 2 +- errorsbp/batch_test.go | 36 +++++++++++++- filewatcher/filewatcher.go | 11 +++-- go.mod | 4 +- go.sum | 4 +- httpbp/config.go | 13 ++--- httpbp/errors.go | 8 ++-- httpbp/server.go | 21 ++++----- .../reddit/baseplate/GoUnusedProtection__.go | 2 +- .../reddit/baseplate/baseplate-consts.go | 7 ++- internal/gen-go/reddit/baseplate/baseplate.go | 43 ++++++++++++----- internal/prometheusbpint/spectest/spec.go | 47 ++++++++----------- kafkabp/rack_test.go | 6 +-- log/wrapper_test.go | 14 +++--- randbp/rand_test.go | 19 ++++---- redis/cache/redisx/syncx.go | 9 ++-- redis/cache/redisx/syncx_test.go | 7 +-- redis/db/redisbp/hooks.go | 9 ++-- retrybp/retry.go | 10 ++-- retrybp/retry_test.go | 25 ++++------ scripts/linters.sh | 12 ++--- secrets/secrets.go | 12 ++--- thriftbp/client_pool.go | 25 +++++----- 25 files changed, 195 insertions(+), 160 deletions(-) diff --git a/.github/workflows/go-ci.yaml b/.github/workflows/go-ci.yaml index 8141f92b5..2750990d2 100644 --- a/.github/workflows/go-ci.yaml +++ b/.github/workflows/go-ci.yaml @@ -13,8 +13,8 @@ jobs: strategy: matrix: go-version: - - "1.19" - "1.20" + - "1.21" container: image: golang:${{ matrix.go-version }} diff --git a/batchcloser/closers.go b/batchcloser/closers.go index 2f298aa6a..7fee7c6b2 100644 --- a/batchcloser/closers.go +++ b/batchcloser/closers.go @@ -2,6 +2,7 @@ package batchcloser import ( "context" + "errors" "fmt" "io" @@ -47,11 +48,11 @@ type BatchCloser struct { // Close implements io.Closer and closes all of it's internal io.Closer objects, // batching any errors into an errorsbp.Batch. func (bc *BatchCloser) Close() error { - var errs errorsbp.Batch + errs := make([]error, 0, len(bc.closers)) for _, closer := range bc.closers { - errs.AddPrefix(fmt.Sprintf("%#v", closer), closer.Close()) + errs = append(errs, errorsbp.Prefix(fmt.Sprintf("%#v", closer), closer.Close())) } - return errs.Compile() + return errors.Join(errs...) } // Add adds the given io.Closer objects to the BatchCloser. diff --git a/errorsbp/batch.go b/errorsbp/batch.go index cbf1e7555..d298b0df1 100644 --- a/errorsbp/batch.go +++ b/errorsbp/batch.go @@ -25,7 +25,7 @@ var ( // This type is not thread-safe. // The same batch should not be operated on different goroutines concurrently. // -// To be deprecated when we drop support for go 1.19. +// Deprecated: Please use errors.Join or fmt.Errorf with multiple %w instead. type Batch struct { errors []error } diff --git a/errorsbp/batch_test.go b/errorsbp/batch_test.go index 42d056267..1b0c5e1f4 100644 --- a/errorsbp/batch_test.go +++ b/errorsbp/batch_test.go @@ -357,8 +357,40 @@ func TestBatchSize(t *testing.T) { nil, // 0 }), }, - // TODO: Add cases from errors.Join and fmt.Errorf once we drop support for - // go 1.19. + { + label: "errors-join", + want: 3, + err: errors.Join( + errors.New("foo"), + errors.New("bar"), + errors.New("fizz"), + ), + }, + { + label: "recursion-errors-join", + want: 5, + err: errors.Join( + nil, // 0 + fmt.Errorf("%w", nil), // 1 + errors.New("foo"), // 1 + simpleBatch{errors.New("foo")}, // 1 + errors.Join( + nil, // 0 + errors.New("foo"), // 1 + errors.New("bar"), // 1 + ), + nil, // 0 + ), + }, + { + label: "fmt.Errorf", + want: 2, + err: fmt.Errorf( + "1: %w, 2: %w", + errors.New("foo"), + errors.New("bar"), + ), + }, } { t.Run(c.label, func(t *testing.T) { if got := errorsbp.BatchSize(c.err); got != c.want { diff --git a/filewatcher/filewatcher.go b/filewatcher/filewatcher.go index 7725b004a..3b7f0c40f 100644 --- a/filewatcher/filewatcher.go +++ b/filewatcher/filewatcher.go @@ -14,7 +14,6 @@ import ( "github.com/fsnotify/fsnotify" - "github.com/reddit/baseplate.go/errorsbp" "github.com/reddit/baseplate.go/internal/limitopen" "github.com/reddit/baseplate.go/log" ) @@ -369,10 +368,12 @@ func New(ctx context.Context, cfg Config) (*Result, error) { select { default: case <-ctx.Done(): - var batch errorsbp.Batch - batch.Add(ctx.Err()) - batch.AddPrefix("last error", lastErr) - return nil, fmt.Errorf("filewatcher: context canceled while waiting for file(s) under %q to load: %w", cfg.Path, batch.Compile()) + return nil, fmt.Errorf( + "filewatcher: context canceled while waiting for file(s) under %q to load: %w, last err: %w", + cfg.Path, + ctx.Err(), + lastErr, + ) } var err error diff --git a/go.mod b/go.mod index 268fb3e56..8883c6c19 100644 --- a/go.mod +++ b/go.mod @@ -1,11 +1,11 @@ module github.com/reddit/baseplate.go -go 1.19 +go 1.20 require ( github.com/Shopify/sarama v1.29.1 github.com/alicebob/miniredis/v2 v2.14.3 - github.com/apache/thrift v0.17.0 + github.com/apache/thrift v0.19.0 github.com/avast/retry-go v3.0.0+incompatible github.com/fsnotify/fsnotify v1.5.4 github.com/getsentry/sentry-go v0.11.0 diff --git a/go.sum b/go.sum index 1ab14a224..7b4c2f1ed 100644 --- a/go.sum +++ b/go.sum @@ -56,8 +56,8 @@ github.com/alicebob/gopher-json v0.0.0-20200520072559-a9ecdc9d1d3a h1:HbKu58rmZp github.com/alicebob/gopher-json v0.0.0-20200520072559-a9ecdc9d1d3a/go.mod h1:SGnFV6hVsYE877CKEZ6tDNTjaSXYUk6QqoIK6PrAtcc= github.com/alicebob/miniredis/v2 v2.14.3 h1:QWoo2wchYmLgOB6ctlTt2dewQ1Vu6phl+iQbwT8SYGo= github.com/alicebob/miniredis/v2 v2.14.3/go.mod h1:gquAfGbzn92jvtrSC69+6zZnwSODVXVpYDRaGhWaL6I= -github.com/apache/thrift v0.17.0 h1:cMd2aj52n+8VoAtvSvLn4kDC3aZ6IAkBuqWQ2IDu7wo= -github.com/apache/thrift v0.17.0/go.mod h1:OLxhMRJxomX+1I/KUw03qoV3mMz16BwaKI+d4fPBx7Q= +github.com/apache/thrift v0.19.0 h1:sOqkWPzMj7w6XaYbJQG7m4sGqVolaW/0D28Ln7yPzMk= +github.com/apache/thrift v0.19.0/go.mod h1:SUALL216IiaOw2Oy+5Vs9lboJ/t9g40C+G07Dc0QC1I= github.com/armon/consul-api v0.0.0-20180202201655-eb2c6b5be1b6/go.mod h1:grANhF5doyWs3UAsr3K4I6qtAmlQcZDesFNEHPZAzj8= github.com/avast/retry-go v3.0.0+incompatible h1:4SOWQ7Qs+oroOTQOYnAHqelpCO0biHSxpiH9JdtuBj0= github.com/avast/retry-go v3.0.0+incompatible/go.mod h1:XtSnn+n/sHqQIpZ10K1qAevBhOOCWBLXXy3hyiqqBrY= diff --git a/httpbp/config.go b/httpbp/config.go index e1b588634..dd7fc0603 100644 --- a/httpbp/config.go +++ b/httpbp/config.go @@ -1,10 +1,11 @@ package httpbp import ( + "errors" + "github.com/avast/retry-go" "github.com/reddit/baseplate.go/breakerbp" - "github.com/reddit/baseplate.go/errorsbp" ) // ClientConfig provides the configuration for a HTTP client including its @@ -19,15 +20,15 @@ type ClientConfig struct { // Validate checks ClientConfig for any missing or erroneous values. func (c ClientConfig) Validate() error { - var batch errorsbp.Batch + var errs []error if c.Slug == "" { - batch.Add(ErrConfigMissingSlug) + errs = append(errs, ErrConfigMissingSlug) } if c.MaxErrorReadAhead < 0 { - batch.Add(ErrConfigInvalidMaxErrorReadAhead) + errs = append(errs, ErrConfigInvalidMaxErrorReadAhead) } if c.MaxConnections < 0 { - batch.Add(ErrConfigInvalidMaxConnections) + errs = append(errs, ErrConfigInvalidMaxConnections) } - return batch.Compile() + return errors.Join(errs...) } diff --git a/httpbp/errors.go b/httpbp/errors.go index db75c9b07..01329233b 100644 --- a/httpbp/errors.go +++ b/httpbp/errors.go @@ -771,9 +771,9 @@ func ClientErrorFromResponse(resp *http.Response) error { // It's required for http response bodies by stdlib http clients to reuse // keep-alive connections, so you should always defer it after checking error. func DrainAndClose(r io.ReadCloser) error { - var batch errorsbp.Batch _, err := io.Copy(io.Discard, r) - batch.Add(err) - batch.Add(r.Close()) - return batch.Compile() + return errors.Join( + errorsbp.Prefix("read fully", err), + errorsbp.Prefix("close", r.Close()), + ) } diff --git a/httpbp/server.go b/httpbp/server.go index b0938df36..113a69e91 100644 --- a/httpbp/server.go +++ b/httpbp/server.go @@ -9,7 +9,6 @@ import ( "sync" "github.com/reddit/baseplate.go" - "github.com/reddit/baseplate.go/errorsbp" //lint:ignore SA1019 This library is internal only, not actually deprecated "github.com/reddit/baseplate.go/internalv2compat" @@ -92,23 +91,23 @@ type Endpoint struct { // Validate checks for input errors on the Endpoint and returns an error // if any exist. func (e Endpoint) Validate() error { - var err errorsbp.Batch + var errs []error if e.Name == "" { - err.Add(errors.New("httpbp: Endpoint.Name must be non-empty")) + errs = append(errs, errors.New("httpbp: Endpoint.Name must be non-empty")) } if e.Handle == nil { - err.Add(errors.New("httpbp: Endpoint.Handle must be non-nil")) + errs = append(errs, errors.New("httpbp: Endpoint.Handle must be non-nil")) } if len(e.Methods) == 0 { - err.Add(errors.New("httpbp: Endpoint.Methods must be non-empty")) + errs = append(errs, errors.New("httpbp: Endpoint.Methods must be non-empty")) } else { for _, method := range e.Methods { if !allHTTPMethods[method] { - err.Add(fmt.Errorf("httpbp: Endpoint.Methods contains an invalid value: %q", method)) + errs = append(errs, fmt.Errorf("httpbp: Endpoint.Methods contains an invalid value: %q", method)) } } } - return err.Compile() + return errors.Join(errs...) } // ServerArgs defines all of the arguments used to create a new HTTP @@ -177,12 +176,12 @@ type ServerArgs struct { // be used for testing purposes. It is called as a part of setting up a new // Baseplate server. func (args ServerArgs) ValidateAndSetDefaults() (ServerArgs, error) { - var inputErrors errorsbp.Batch + var errs []error if args.Baseplate == nil { - inputErrors.Add(errors.New("argument Baseplate must be non-nil")) + errs = append(errs, errors.New("argument Baseplate must be non-nil")) } for _, endpoint := range args.Endpoints { - inputErrors.Add(endpoint.Validate()) + errs = append(errs, endpoint.Validate()) } if args.EndpointRegistry == nil { args.EndpointRegistry = http.NewServeMux() @@ -190,7 +189,7 @@ func (args ServerArgs) ValidateAndSetDefaults() (ServerArgs, error) { if args.TrustHandler == nil { args.TrustHandler = NeverTrustHeaders{} } - return args, inputErrors.Compile() + return args, errors.Join(errs...) } // SetupEndpoints calls ValidateAndSetDefaults and registeres the Endpoints diff --git a/internal/gen-go/reddit/baseplate/GoUnusedProtection__.go b/internal/gen-go/reddit/baseplate/GoUnusedProtection__.go index a470921a4..ea1c598df 100644 --- a/internal/gen-go/reddit/baseplate/GoUnusedProtection__.go +++ b/internal/gen-go/reddit/baseplate/GoUnusedProtection__.go @@ -1,4 +1,4 @@ -// Code generated by Thrift Compiler (0.17.0). DO NOT EDIT. +// Code generated by Thrift Compiler (0.19.0). DO NOT EDIT. package baseplate diff --git a/internal/gen-go/reddit/baseplate/baseplate-consts.go b/internal/gen-go/reddit/baseplate/baseplate-consts.go index 825262d7b..e879ea8b7 100644 --- a/internal/gen-go/reddit/baseplate/baseplate-consts.go +++ b/internal/gen-go/reddit/baseplate/baseplate-consts.go @@ -1,4 +1,4 @@ -// Code generated by Thrift Compiler (0.17.0). DO NOT EDIT. +// Code generated by Thrift Compiler (0.19.0). DO NOT EDIT. package baseplate @@ -9,6 +9,8 @@ import ( "fmt" "time" thrift "github.com/apache/thrift/lib/go/thrift" + "strings" + "regexp" ) // (needed to ensure safety because of naive import list construction.) @@ -18,6 +20,9 @@ var _ = errors.New var _ = context.Background var _ = time.Now var _ = bytes.Equal +// (needed by validator.) +var _ = strings.Contains +var _ = regexp.MatchString func init() { diff --git a/internal/gen-go/reddit/baseplate/baseplate.go b/internal/gen-go/reddit/baseplate/baseplate.go index 5bcb8eb15..2b0afc7d0 100644 --- a/internal/gen-go/reddit/baseplate/baseplate.go +++ b/internal/gen-go/reddit/baseplate/baseplate.go @@ -1,4 +1,4 @@ -// Code generated by Thrift Compiler (0.17.0). DO NOT EDIT. +// Code generated by Thrift Compiler (0.19.0). DO NOT EDIT. package baseplate @@ -10,6 +10,8 @@ import ( "fmt" "time" thrift "github.com/apache/thrift/lib/go/thrift" + "strings" + "regexp" ) // (needed to ensure safety because of naive import list construction.) @@ -19,6 +21,9 @@ var _ = errors.New var _ = context.Background var _ = time.Now var _ = bytes.Equal +// (needed by validator.) +var _ = strings.Contains +var _ = regexp.MatchString //The different types of probes supported by is_healthy endpoint. // @@ -343,6 +348,9 @@ func (p *IsHealthyRequest) String() string { return fmt.Sprintf("IsHealthyRequest(%+v)", *p) } +func (p *IsHealthyRequest) Validate() error { + return nil +} // Attributes: // - Code: A code describing the general nature of the error. // This should be specified for all errors. This field uses @@ -666,6 +674,9 @@ func (Error) TExceptionType() thrift.TExceptionType { var _ thrift.TException = (*Error)(nil) +func (p *Error) Validate() error { + return nil +} type BaseplateService interface { //The base for any baseplate-based service. // //Your service should inherit from this one so that common tools can interact @@ -810,13 +821,13 @@ func (p *baseplateServiceProcessorIsHealthy) Process(ctx context.Context, seqId tickerCancel := func() {} // Start a goroutine to do server side connectivity check. if thrift.ServerConnectivityCheckInterval > 0 { - var cancel context.CancelFunc - ctx, cancel = context.WithCancel(ctx) - defer cancel() + var cancel context.CancelCauseFunc + ctx, cancel = context.WithCancelCause(ctx) + defer cancel(nil) var tickerCtx context.Context tickerCtx, tickerCancel = context.WithCancel(context.Background()) defer tickerCancel() - go func(ctx context.Context, cancel context.CancelFunc) { + go func(ctx context.Context, cancel context.CancelCauseFunc) { ticker := time.NewTicker(thrift.ServerConnectivityCheckInterval) defer ticker.Stop() for { @@ -825,7 +836,7 @@ func (p *baseplateServiceProcessorIsHealthy) Process(ctx context.Context, seqId return case <-ticker.C: if !iprot.Transport().IsOpen() { - cancel() + cancel(thrift.ErrAbandonRequest) return } } @@ -840,6 +851,11 @@ func (p *baseplateServiceProcessorIsHealthy) Process(ctx context.Context, seqId if errors.Is(err2, thrift.ErrAbandonRequest) { return false, thrift.WrapTException(err2) } + if errors.Is(err2, context.Canceled) { + if err := context.Cause(ctx); errors.Is(err, thrift.ErrAbandonRequest) { + return false, thrift.WrapTException(err) + } + } _exc9 := thrift.NewTApplicationException(thrift.INTERNAL_ERROR, "Internal error processing is_healthy: " + err2.Error()) if err2 := oprot.WriteMessageBegin(ctx, "is_healthy", thrift.EXCEPTION, seqId); err2 != nil { _write_err8 = thrift.WrapTException(err2) @@ -1181,13 +1197,13 @@ func (p *baseplateServiceV2ProcessorIsHealthy) Process(ctx context.Context, seqI tickerCancel := func() {} // Start a goroutine to do server side connectivity check. if thrift.ServerConnectivityCheckInterval > 0 { - var cancel context.CancelFunc - ctx, cancel = context.WithCancel(ctx) - defer cancel() + var cancel context.CancelCauseFunc + ctx, cancel = context.WithCancelCause(ctx) + defer cancel(nil) var tickerCtx context.Context tickerCtx, tickerCancel = context.WithCancel(context.Background()) defer tickerCancel() - go func(ctx context.Context, cancel context.CancelFunc) { + go func(ctx context.Context, cancel context.CancelCauseFunc) { ticker := time.NewTicker(thrift.ServerConnectivityCheckInterval) defer ticker.Stop() for { @@ -1196,7 +1212,7 @@ func (p *baseplateServiceV2ProcessorIsHealthy) Process(ctx context.Context, seqI return case <-ticker.C: if !iprot.Transport().IsOpen() { - cancel() + cancel(thrift.ErrAbandonRequest) return } } @@ -1211,6 +1227,11 @@ func (p *baseplateServiceV2ProcessorIsHealthy) Process(ctx context.Context, seqI if errors.Is(err2, thrift.ErrAbandonRequest) { return false, thrift.WrapTException(err2) } + if errors.Is(err2, context.Canceled) { + if err := context.Cause(ctx); errors.Is(err, thrift.ErrAbandonRequest) { + return false, thrift.WrapTException(err) + } + } _exc16 := thrift.NewTApplicationException(thrift.INTERNAL_ERROR, "Internal error processing is_healthy: " + err2.Error()) if err2 := oprot.WriteMessageBegin(ctx, "is_healthy", thrift.EXCEPTION, seqId); err2 != nil { _write_err15 = thrift.WrapTException(err2) diff --git a/internal/prometheusbpint/spectest/spec.go b/internal/prometheusbpint/spectest/spec.go index bcfc4f78e..4c7771da2 100644 --- a/internal/prometheusbpint/spectest/spec.go +++ b/internal/prometheusbpint/spectest/spec.go @@ -9,8 +9,6 @@ import ( "github.com/google/go-cmp/cmp" "github.com/prometheus/client_golang/prometheus" "github.com/prometheus/client_golang/prometheus/testutil" - - "github.com/reddit/baseplate.go/errorsbp" ) const ( @@ -40,25 +38,21 @@ var ( func ValidateSpec(tb testing.TB, metricPrefix, clientOrServer string) { tb.Helper() - var batch errorsbp.Batch - batch.AddPrefix("validate spec "+clientOrServer, validateSpec(metricPrefix, clientOrServer)) - - if err := batch.Compile(); err != nil { - tb.Error(err) + if err := validateSpec(metricPrefix, clientOrServer); err != nil { + tb.Errorf("validate spec %s: %v", clientOrServer, err) } } func validateSpec(prefix, clientOrServer string) error { - var batch errorsbp.Batch - metricFam, err := prometheus.DefaultGatherer.Gather() if err != nil { - batch.Add(err) - return batch.Compile() + return err } metricsUnderTest := buildMetricNames(prefix, clientOrServer) + var errs []error + for _, m := range metricFam { name := *m.Name @@ -66,8 +60,8 @@ func validateSpec(prefix, clientOrServer string) error { continue } - batch.Add(lintMetric(name)) - batch.Add(validateName(name, prefix, clientOrServer)) + errs = append(errs, lintMetric(name)) + errs = append(errs, validateName(name, prefix, clientOrServer)) labels := make(map[string]struct{}) for _, metric := range m.GetMetric() { @@ -75,7 +69,7 @@ func validateSpec(prefix, clientOrServer string) error { labels[*label.Name] = struct{}{} } } - batch.Add(validateLabels(name, prefix, clientOrServer, labels)) + errs = append(errs, validateLabels(name, prefix, clientOrServer, labels)) // after we validate the metric, delete it from the set of all // expected metrics so that we can track if any were not found. @@ -83,10 +77,10 @@ func validateSpec(prefix, clientOrServer string) error { } if len(metricsUnderTest) > 0 { - batch.Add(fmt.Errorf("%w: %v", errNotFound, keysFrom(metricsUnderTest))) + errs = append(errs, fmt.Errorf("%w: %v", errNotFound, keysFrom(metricsUnderTest))) } - return batch.Compile() + return errors.Join(errs...) } func keysFrom(metrics map[string]struct{}) []string { @@ -116,24 +110,23 @@ func buildMetricNames(prefix, clientOrServer string) map[string]struct{} { // 2. the metric name has the correct prefix. // 3. the metric contains either "client" or "server" as the second part. func validateName(name, prefix, clientOrServer string) error { - var batch errorsbp.Batch - const separator = "_" parts := strings.SplitN(name, separator, partsCount) if len(parts) < partsCount { - batch.Add(fmt.Errorf("%w: name: %q part count: %d", errLength, name, len(parts))) - return batch.Compile() + return fmt.Errorf("%w: name: %q part count: %d", errLength, name, len(parts)) } + var errs []error + if got, want := parts[0], prefix; got != want { - batch.Add(fmt.Errorf("%w: name: %q prefix: %q", errPrefix, name, prefix)) + errs = append(errs, fmt.Errorf("%w: name: %q prefix: %q", errPrefix, name, prefix)) } if got, want := parts[1], clientOrServer; got != want { - batch.Add(fmt.Errorf("%w: name: %q", errClientServer, name)) + errs = append(errs, fmt.Errorf("%w: name: %q", errClientServer, name)) } - return batch.Compile() + return errors.Join(errs...) } func validateLabels(name, prefix, clientOrServer string, gotLabels map[string]struct{}) error { @@ -264,13 +257,13 @@ func httpSpecificLabels(name, clientOrServer string) []string { } func lintMetric(metricName string) error { - var batch errorsbp.Batch + var errs []error problems, err := testutil.GatherAndLint(prometheus.DefaultGatherer, metricName) if err != nil { - batch.Add(err) + errs = append(errs, err) } for _, p := range problems { - batch.Add(fmt.Errorf("%w: name: %q %s", errPrometheusLint, metricName, p.Text)) + errs = append(errs, fmt.Errorf("%w: name: %q %s", errPrometheusLint, metricName, p.Text)) } - return batch.Compile() + return errors.Join(errs...) } diff --git a/kafkabp/rack_test.go b/kafkabp/rack_test.go index b5664ec28..6aa3637e1 100644 --- a/kafkabp/rack_test.go +++ b/kafkabp/rack_test.go @@ -22,15 +22,15 @@ func TestRackIDFuncUnmarshalText(t *testing.T) { }{ { text: "aws", - expected: "kafkabp.AWSAvailabilityZoneRackID", + expected: "AWSAvailabilityZoneRackID", }, { text: "http://www.google.com", - expected: "kafkabp.SimpleHTTPRackID", + expected: "SimpleHTTPRackID", }, { text: "https://www.google.com", - expected: "kafkabp.SimpleHTTPRackID", + expected: "SimpleHTTPRackID", }, /* Starting from go 1.17, the FixedRackID function starts to be inlined diff --git a/log/wrapper_test.go b/log/wrapper_test.go index 285d0e60d..3f22765cc 100644 --- a/log/wrapper_test.go +++ b/log/wrapper_test.go @@ -38,25 +38,25 @@ func TestLogWrapperUnmarshalText(t *testing.T) { }, { text: "", - expected: "log.ErrorWithSentryWrapper", + expected: "ErrorWithSentryWrapper", }, { text: "nop", - expected: "log.NopWrapper", + expected: "NopWrapper", }, { text: "std", - expected: "log.StdWrapper", + expected: "StdWrapper", }, { text: "zap", - expected: "log.ZapWrapper", + expected: "ZapWrapper", }, { // Unfortunately there's no way to check that the arg passed into // ZapWrapper is correct. text: "zap:error", - expected: "log.ZapWrapper", + expected: "ZapWrapper", }, { text: "zap:error:key", @@ -68,7 +68,7 @@ func TestLogWrapperUnmarshalText(t *testing.T) { }, { text: "zap:info:key1=value1,key2=value2 with space", - expected: "log.ZapWrapper", + expected: "ZapWrapper", }, { text: "zaperror", @@ -76,7 +76,7 @@ func TestLogWrapperUnmarshalText(t *testing.T) { }, { text: "sentry", - expected: "log.ErrorWithSentryWrapper", + expected: "ErrorWithSentryWrapper", }, } { t.Run(c.text, func(t *testing.T) { diff --git a/randbp/rand_test.go b/randbp/rand_test.go index 72e5fa6ea..d1b5b253e 100644 --- a/randbp/rand_test.go +++ b/randbp/rand_test.go @@ -18,14 +18,14 @@ func swap(slice []byte) func(i, j int) { } } -func seedBoth() { +func seedBoth() *rand.Rand { seed := randbp.GetSeed() - rand.Seed(seed) randbp.R.Seed(seed) + return rand.New(rand.NewSource(seed)) } func TestSameResult(t *testing.T) { - seedBoth() + r := seedBoth() const epsilon = 1e-9 equalFloat64 := func(a, b float64) bool { @@ -36,7 +36,7 @@ func TestSameResult(t *testing.T) { "Uint64", func(t *testing.T) { f := func() bool { - u64math := rand.Uint64() + u64math := r.Uint64() u64bp := randbp.R.Uint64() if u64math != u64bp { t.Errorf( @@ -57,7 +57,7 @@ func TestSameResult(t *testing.T) { "Float64", func(t *testing.T) { f := func() bool { - f64math := rand.Float64() + f64math := r.Float64() f64bp := randbp.R.Float64() if !equalFloat64(f64math, f64bp) { t.Errorf( @@ -78,7 +78,7 @@ func TestSameResult(t *testing.T) { "NormFloat64", func(t *testing.T) { f := func() bool { - f64math := rand.NormFloat64() + f64math := r.NormFloat64() f64bp := randbp.R.NormFloat64() if !equalFloat64(f64math, f64bp) { t.Errorf( @@ -99,7 +99,7 @@ func TestSameResult(t *testing.T) { "ExpFloat64", func(t *testing.T) { f := func() bool { - f64math := rand.ExpFloat64() + f64math := r.ExpFloat64() f64bp := randbp.R.ExpFloat64() if !equalFloat64(f64math, f64bp) { t.Errorf( @@ -124,7 +124,7 @@ func TestSameResult(t *testing.T) { bpSlice := make([]byte, len(s)) copy(mathSlice, []byte(s)) copy(bpSlice, []byte(s)) - rand.Shuffle(len(s), swap(mathSlice)) + r.Shuffle(len(s), swap(mathSlice)) randbp.R.Shuffle(len(s), swap(bpSlice)) if !reflect.DeepEqual(mathSlice, bpSlice) { t.Errorf( @@ -146,7 +146,7 @@ func TestSameResult(t *testing.T) { func(t *testing.T) { f := func(smalln uint8) bool { n := int(smalln) - mathSlice := rand.Perm(n) + mathSlice := r.Perm(n) bpSlice := randbp.R.Perm(n) if !reflect.DeepEqual(mathSlice, bpSlice) { t.Errorf( @@ -181,6 +181,7 @@ func BenchmarkRand(b *testing.B) { b.RunParallel(func(pb *testing.PB) { buf := make([]byte, size) for pb.Next() { + //lint:ignore SA1019 We intentionally want to compare the performce against rand.Read here rand.Read(buf) } }) diff --git a/redis/cache/redisx/syncx.go b/redis/cache/redisx/syncx.go index eefde7a89..87c89182c 100644 --- a/redis/cache/redisx/syncx.go +++ b/redis/cache/redisx/syncx.go @@ -2,10 +2,9 @@ package redisx import ( "context" + "errors" "github.com/joomcode/redispipe/redis" - - "github.com/reddit/baseplate.go/errorsbp" ) // Syncx is a wrapper around a Sync that provides an API that uses reflection to @@ -127,11 +126,11 @@ func (s Syncx) SendTransaction(ctx context.Context, reqs ...Request) error { if err != nil { return err } - var errs errorsbp.Batch + errs := make([]error, 0, len(results)) for i, res := range results { - errs.Add(reqs[i].setValue(res)) + errs = append(errs, reqs[i].setValue(res)) } - return errs.Compile() + return errors.Join(errs...) } // Scanner returns a ScanIterator from the underlying Sync. diff --git a/redis/cache/redisx/syncx_test.go b/redis/cache/redisx/syncx_test.go index 6ecfc0a58..9cde58edd 100644 --- a/redis/cache/redisx/syncx_test.go +++ b/redis/cache/redisx/syncx_test.go @@ -10,7 +10,6 @@ import ( "github.com/joomcode/errorx" "github.com/joomcode/redispipe/redis" - "github.com/reddit/baseplate.go/errorsbp" "github.com/reddit/baseplate.go/redis/cache/redisx" ) @@ -107,10 +106,8 @@ func TestSyncx_SendMany(t *testing.T) { redisx.Req(&v1, "PING"), redisx.Req(&v2, "SET", "k", "v"), ) - batch := errorsbp.Batch{} - batch.Add(errs...) - if batch.Compile() != nil { - t.Fatalf("expected no errors, got %+v", errs) + if err := errors.Join(errs...); err != nil { + t.Fatalf("expected no errors, got %v", err) } if v1 != pong { t.Errorf("v1 did not match, expected %q, got %q", pong, v1) diff --git a/redis/db/redisbp/hooks.go b/redis/db/redisbp/hooks.go index 8a1242574..963a9dad3 100644 --- a/redis/db/redisbp/hooks.go +++ b/redis/db/redisbp/hooks.go @@ -10,7 +10,6 @@ import ( "github.com/opentracing/opentracing-go" "github.com/prometheus/client_golang/prometheus" - "github.com/reddit/baseplate.go/errorsbp" "github.com/reddit/baseplate.go/internal/prometheusbpint" "github.com/reddit/baseplate.go/prometheusbp" "github.com/reddit/baseplate.go/redis/internal/redisprom" @@ -67,13 +66,13 @@ func (h SpanHook) BeforeProcessPipeline(ctx context.Context, cmds []redis.Cmder) // publishes the time the Redis pipeline took to complete, and a metric // indicating whether the pipeline was a "success" or "fail" func (h SpanHook) AfterProcessPipeline(ctx context.Context, cmds []redis.Cmder) error { - var errs errorsbp.Batch + errs := make([]error, 0, len(cmds)) for _, cmd := range cmds { - if !errors.Is(cmd.Err(), redis.Nil) { - errs.Add(cmd.Err()) + if err := cmd.Err(); !errors.Is(err, redis.Nil) { + errs = append(errs, err) } } - h.endChildSpan(ctx, errs.Compile()) + h.endChildSpan(ctx, errors.Join(errs...)) // NOTE: returning non-nil error from the hook changes the error the caller gets, and that's something we want to avoid. // see: https://github.com/go-redis/redis/blob/v8.10.0/redis.go#L101 return nil diff --git a/retrybp/retry.go b/retrybp/retry.go index 2203ea611..fcd29ce08 100644 --- a/retrybp/retry.go +++ b/retrybp/retry.go @@ -6,8 +6,6 @@ import ( "time" "github.com/avast/retry-go" - - "github.com/reddit/baseplate.go/errorsbp" ) func init() { @@ -46,8 +44,8 @@ func GetOptions(ctx context.Context) (options []retry.Option, ok bool) { // where you are not calling Do directly but still want to be able to configure // retry behavior per-call. // -// 2. If retry.Do returns a batch of errors (retry.Error), put those in a -// errorsbp.Batch from baseplate.go. +// 2. If retry.Do returns a batch of errors (retry.Error), returns an error +// containing all of them. // // It also auto applies retry.Context with the ctx given, // so that the retries will be stopped as soon as ctx is canceled. @@ -64,9 +62,7 @@ func Do(ctx context.Context, fn func() error, defaults ...retry.Option) error { var retryErr retry.Error if errors.As(err, &retryErr) { - var batchErr errorsbp.Batch - batchErr.Add(retryErr.WrappedErrors()...) - return batchErr.Compile() + return errors.Join(retryErr.WrappedErrors()...) } return err diff --git a/retrybp/retry_test.go b/retrybp/retry_test.go index 69f4f78af..c4d8f11fc 100644 --- a/retrybp/retry_test.go +++ b/retrybp/retry_test.go @@ -94,23 +94,18 @@ func TestDoBatchError(t *testing.T) { retry.Attempts(uint(len(errList))), ) - var batchErr errorsbp.Batch - if errors.As(err, &batchErr) { - if len(batchErr.GetErrors()) != len(errList) { - t.Errorf( - "wrong number of errors in %v, expected %v", - batchErr.GetErrors(), - errList, - ) - - for _, err := range errList { - if !errors.Is(batchErr, err) { - t.Errorf("%v is not wrapped by %v", err, batchErr) - } + if got, want := errorsbp.BatchSize(err), len(errList); got != want { + t.Errorf( + "wrong number of errors in %v, expected %v", + err, + errList, + ) + + for _, e := range errList { + if !errors.Is(err, e) { + t.Errorf("%v is not wrapped by %v", e, err) } } - } else { - t.Fatalf("unexpected error type %v", err) } } diff --git a/scripts/linters.sh b/scripts/linters.sh index 892d62288..fccc07b99 100755 --- a/scripts/linters.sh +++ b/scripts/linters.sh @@ -7,8 +7,8 @@ FILES=$(find . -name "*.go" | grep -v -e "\/gen-go\/") FAILED=0 for FILE in $FILES; do - FMT=$(gofmt -s -d "$FILE") - if [ -n "$FMT" ]; then + FMT=$(gofmt -s -d "$FILE" 2>&1) + if [ $? -ne 0 ]; then echo "gofmt:" echo "$FILE:" echo "$FMT" @@ -16,15 +16,15 @@ for FILE in $FILES; do fi done -VET=$(go vet ./...) -if [ -n "$VET" ]; then +VET=$(go vet ./... 2>&1) +if [ $? -ne 0 ]; then echo "go vet:" echo "$VET" FAILED=1 fi -STATICCHECK=$(staticcheck ./...) -if [ -n "$STATICCHECK" ]; then +STATICCHECK=$(staticcheck ./... 2>&1) +if [ $? -ne 0 ]; then echo "$(staticcheck --version):" echo "$STATICCHECK" FAILED=1 diff --git a/secrets/secrets.go b/secrets/secrets.go index cb05858be..400baf931 100644 --- a/secrets/secrets.go +++ b/secrets/secrets.go @@ -7,8 +7,6 @@ import ( "io" "io/fs" "path/filepath" - - "github.com/reddit/baseplate.go/errorsbp" ) const ( @@ -200,28 +198,28 @@ type Document struct { // When this function returns a non-nil error, the error is either a // TooManyFieldsError, or a BatchError containing multiple TooManyFieldsError. func (s *Document) Validate() error { - var batch errorsbp.Batch + errs := make([]error, 0, len(s.Secrets)) for key, value := range s.Secrets { if value.Type == SimpleType && notOnlySimpleSecret(value) { - batch.Add(TooManyFieldsError{ + errs = append(errs, TooManyFieldsError{ SecretType: SimpleType, Key: key, }) } if value.Type == VersionedType && notOnlyVersionedSecret(value) { - batch.Add(TooManyFieldsError{ + errs = append(errs, TooManyFieldsError{ SecretType: VersionedType, Key: key, }) } if value.Type == CredentialType && notOnlyCredentialSecret(value) { - batch.Add(TooManyFieldsError{ + errs = append(errs, TooManyFieldsError{ SecretType: CredentialType, Key: key, }) } } - return batch.Compile() + return errors.Join(errs...) } func notOnlySimpleSecret(secret GenericSecret) bool { diff --git a/thriftbp/client_pool.go b/thriftbp/client_pool.go index eda4b8ef1..1f307d921 100644 --- a/thriftbp/client_pool.go +++ b/thriftbp/client_pool.go @@ -218,11 +218,10 @@ type ClientPoolConfig struct { // If this is the configuration for a baseplate service // BaseplateClientPoolConfig(c).Validate should be used instead. func (c ClientPoolConfig) Validate() error { - var batch errorsbp.Batch if c.InitialConnections > c.MaxConnections { - batch.Add(ErrConfigInvalidConnections) + return ErrConfigInvalidConnections } - return batch.Compile() + return nil } var tHeaderProtocolCompact = thrift.THeaderProtocolIDPtrMust(thrift.THeaderProtocolCompact) @@ -249,17 +248,17 @@ type BaseplateClientPoolConfig ClientPoolConfig // This method is designated to be used when passing a configuration to // NewBaseplateClientPool, for NewCustomClientPool other constraints apply. func (c BaseplateClientPoolConfig) Validate() error { - var batch errorsbp.Batch + var errs []error if c.ServiceSlug == "" { - batch.Add(ErrConfigMissingServiceSlug) + errs = append(errs, ErrConfigMissingServiceSlug) } if c.Addr == "" { - batch.Add(ErrConfigMissingAddr) + errs = append(errs, ErrConfigMissingAddr) } if c.InitialConnections > c.MaxConnections { - batch.Add(ErrConfigInvalidConnections) + errs = append(errs, ErrConfigInvalidConnections) } - return batch.Compile() + return errors.Join(errs...) } // Client is a client object that implements both the clientpool.Client and @@ -490,15 +489,13 @@ func newClientPool( // Register should never fail because clientPoolGaugeExporter.Describe is // a no-op, but just in case. - var batch errorsbp.Batch - batch.Add(err) - if err := pool.Close(); err != nil { - batch.AddPrefix("close pool", err) - } return nil, fmt.Errorf( "thriftbp: error registering prometheus exporter for client pool %q: %w", cfg.ServiceSlug, - batch.Compile(), + errors.Join( + err, + errorsbp.Prefix("close pool", pool.Close()), + ), ) }