From 95c078384496c380e2791eb72d1568e27656b825 Mon Sep 17 00:00:00 2001 From: Albert Wu Date: Tue, 21 Jul 2026 16:45:46 -0700 Subject: [PATCH] feat(errs): share cross-domain error identities Summary: Define ErrNotFound and ErrVersionMismatch in platform/errs for conditions with stable semantics across domains. Migrate SubmitQueue and Stovepipe storage and queue configuration code to use the platform errors directly, and keep only ErrVersionMismatch automatically classified as retryable. Test Plan: - make fmt - make build - make test - make lint - make check-tidy - make check-gazelle Revert Plan: Revert this commit. API Changes: Remove domain ErrNotFound and ErrVersionMismatch sentinels plus storage IsNotFound and WrapNotFound helpers. Callers use platform/errs directly. Monitoring and Alerts: N/A --- CLAUDE.md | 4 +-- doc/rfc/stovepipe/steps/build.md | 2 +- platform/errs/README.md | 23 +++++++------- platform/errs/errs.go | 7 +++++ platform/errs/generic/generic.go | 4 +++ platform/errs/generic/generic_test.go | 30 +++++++++++++++++-- stovepipe/controller/BUILD.bazel | 1 + stovepipe/controller/dlq/BUILD.bazel | 3 +- stovepipe/controller/dlq/dlq.go | 7 +++-- stovepipe/controller/dlq/dlq_test.go | 8 ++--- stovepipe/controller/ingest.go | 8 ++--- stovepipe/controller/ingest_test.go | 21 ++++++------- stovepipe/controller/process/BUILD.bazel | 1 - stovepipe/controller/process/process.go | 16 +++++----- stovepipe/controller/process/process_test.go | 19 ++++++------ .../extension/queueconfig/default/BUILD.bazel | 4 +-- .../extension/queueconfig/default/default.go | 4 +-- .../queueconfig/default/default_test.go | 4 +-- .../extension/queueconfig/queueconfig.go | 6 +--- stovepipe/extension/storage/build_store.go | 4 +-- stovepipe/extension/storage/mysql/BUILD.bazel | 2 ++ .../extension/storage/mysql/build_store.go | 9 +++--- .../storage/mysql/build_store_test.go | 5 ++-- .../extension/storage/mysql/queue_store.go | 9 +++--- .../storage/mysql/queue_store_test.go | 5 ++-- .../extension/storage/mysql/request_store.go | 9 +++--- .../storage/mysql/request_store_test.go | 5 ++-- .../storage/mysql/request_uri_store.go | 5 ++-- .../storage/mysql/request_uri_store_test.go | 3 +- stovepipe/extension/storage/queue_store.go | 4 +-- stovepipe/extension/storage/request_store.go | 4 +-- .../extension/storage/request_uri_store.go | 2 +- stovepipe/extension/storage/storage.go | 19 ------------ submitqueue/core/request/BUILD.bazel | 3 +- submitqueue/core/request/materializer.go | 7 +++-- submitqueue/core/request/materializer_test.go | 8 ++--- submitqueue/core/request/request_test.go | 6 ++-- .../extension/queueconfig/queueconfig.go | 6 +--- .../extension/queueconfig/yaml/BUILD.bazel | 4 +-- .../extension/queueconfig/yaml/yaml.go | 6 ++-- .../extension/queueconfig/yaml/yaml_test.go | 4 +-- submitqueue/extension/storage/README.md | 2 +- .../storage/batch_dependent_store.go | 6 ++-- submitqueue/extension/storage/batch_store.go | 6 ++-- submitqueue/extension/storage/build_store.go | 2 +- .../extension/storage/mysql/BUILD.bazel | 2 ++ .../storage/mysql/batch_dependent_store.go | 9 +++--- .../mysql/batch_dependent_store_test.go | 5 ++-- .../extension/storage/mysql/batch_store.go | 13 ++++---- .../storage/mysql/batch_store_test.go | 7 +++-- .../extension/storage/mysql/build_store.go | 9 +++--- .../storage/mysql/build_store_test.go | 5 ++-- .../storage/mysql/request_log_store.go | 3 +- .../storage/mysql/request_log_store_test.go | 3 +- .../mysql/request_queue_summary_store.go | 5 ++-- .../mysql/request_queue_summary_store_test.go | 5 ++-- .../extension/storage/mysql/request_store.go | 9 +++--- .../storage/mysql/request_store_test.go | 5 ++-- .../storage/mysql/request_summary_store.go | 5 ++-- .../mysql/request_summary_store_test.go | 5 ++-- .../extension/storage/request_log_store.go | 2 +- .../storage/request_queue_summary_store.go | 4 +-- .../extension/storage/request_store.go | 4 +-- .../storage/request_summary_store.go | 4 +-- submitqueue/extension/storage/storage.go | 19 ------------ submitqueue/gateway/controller/BUILD.bazel | 1 - submitqueue/gateway/controller/cancel.go | 3 +- submitqueue/gateway/controller/land.go | 2 +- submitqueue/gateway/controller/land_test.go | 5 ++-- submitqueue/gateway/controller/list.go | 2 +- submitqueue/gateway/controller/list_test.go | 4 +-- .../gateway/controller/request_history.go | 5 ++-- .../controller/request_history_test.go | 7 ++--- .../gateway/controller/request_summary.go | 5 ++-- .../controller/request_summary_test.go | 5 ++-- .../controller/storage_fixture_test.go | 13 ++++---- .../orchestrator/controller/batch/BUILD.bazel | 3 +- .../orchestrator/controller/batch/batch.go | 5 ++-- .../controller/batch/batch_test.go | 6 ++-- .../controller/cancel/BUILD.bazel | 1 + .../orchestrator/controller/cancel/cancel.go | 10 +++---- .../controller/cancel/cancel_test.go | 21 ++++++------- .../controller/conclude/BUILD.bazel | 1 - .../controller/conclude/conclude_test.go | 3 +- .../orchestrator/controller/dlq/BUILD.bazel | 2 +- .../orchestrator/controller/dlq/README.md | 4 +-- .../controller/dlq/buildsignal.go | 3 +- .../controller/dlq/buildsignal_test.go | 4 +-- .../orchestrator/controller/dlq/dlq.go | 5 ++-- .../orchestrator/controller/dlq/dlq_test.go | 5 ++-- .../controller/speculate/BUILD.bazel | 2 +- .../controller/speculate/speculate.go | 5 ++-- .../controller/speculate/speculate_test.go | 13 ++++---- .../controller/validate/BUILD.bazel | 1 - .../controller/validate/validate.go | 2 +- .../controller/validate/validate_test.go | 3 +- .../stovepipe/extension/storage/BUILD.bazel | 1 + .../extension/storage/mysql/BUILD.bazel | 1 + .../extension/storage/mysql/storage_test.go | 9 +++--- .../stovepipe/extension/storage/suite.go | 13 ++++---- .../submitqueue/extension/storage/BUILD.bazel | 1 + .../submitqueue/extension/storage/suite.go | 13 ++++---- 102 files changed, 328 insertions(+), 301 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 941840a8..96f15480 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -339,6 +339,6 @@ Errors are classified by origin (user vs infra) and retryability. The framework **Key rules:** 1. **Non-retryable by default** — a plain `fmt.Errorf(...)` is non-retryable. Retryability is opted into explicitly, but that decision is almost always made by a classifier, not a controller (see rule 4). 2. **Infra by default** — any error not wrapped with `NewUserError` is infra. There is no `NewInfraError`. -3. **Extensions return plain errors** — extension interfaces (`MergeChecker`, `Storage`, `Publisher`) return standard `error` values with their own domain sentinels (e.g. `storage.ErrNotFound`). They do NOT classify errors as user or infra. -4. **Classifiers do the bulk of classification; controllers override only with knowledge a classifier lacks** — primary pipeline consumers compose per-backend classifiers into `errs.NewClassifierProcessor(...)`; the processor runs once per chain in the consumer and decides retryability from the raw error. So the common case is a controller returning the raw error (`fmt.Errorf("...: %w", err)`) and letting the classifier verdict stand. Reserve an explicit `errs.New*Error` wrap for the rare case where the controller knows something the classifier cannot infer from the error value alone (e.g. `storage.ErrNotFound` meaning "user asked for a missing resource" *in this call site*). Do **not** wrap a failure as retryable just because replaying it is convenient (e.g. a failed queue publish) — that turns permanent failures into infinite retries instead of dead-lettering. DLQ reconciliation consumers use `errs.AlwaysRetryableProcessor` instead. See [platform/errs/README.md](platform/errs/README.md). +3. **Extensions return plain errors** - extension interfaces (`MergeChecker`, `Storage`, `Publisher`) return standard `error` values. They use shared `platform/errs` sentinels for conditions with common cross-domain semantics and domain sentinels for domain-specific conditions. They do NOT classify errors as user or infra. +4. **Classifiers do the bulk of classification; controllers override only with knowledge a classifier lacks** - primary pipeline consumers compose per-backend classifiers into `errs.NewClassifierProcessor(...)`; the processor runs once per chain in the consumer and decides retryability from the raw error. So the common case is a controller returning the raw error (`fmt.Errorf("...: %w", err)`) and letting the classifier verdict stand. Reserve an explicit `errs.New*Error` wrap for the rare case where the controller knows something the classifier cannot infer from the error value alone (e.g. `errs.ErrNotFound` meaning "user asked for a missing resource" *in this call site*). Do **not** wrap a failure as retryable just because replaying it is convenient (e.g. a failed queue publish) - that turns permanent failures into infinite retries instead of dead-lettering. DLQ reconciliation consumers use `errs.AlwaysRetryableProcessor` instead. See [platform/errs/README.md](platform/errs/README.md). 5. **Error chain works end-to-end** — extensions wrap custom errors, controllers wrap with `errs.New*Error`, and `errors.Is`/`errors.As` walks the full chain. diff --git a/doc/rfc/stovepipe/steps/build.md b/doc/rfc/stovepipe/steps/build.md index df673235..cccf01b9 100644 --- a/doc/rfc/stovepipe/steps/build.md +++ b/doc/rfc/stovepipe/steps/build.md @@ -101,7 +101,7 @@ Per `platform/errs`'s non-retryable-by-default rule (see [platform/errs/README.m | Failure | Disposition | Why | |---|---|---| -| `Request` not found (`storage.ErrNotFound`) | retryable (`errs.NewRetryableError`) | On a primary-only read this shouldn't happen in practice — `process` commits before it publishes — but the check costs nothing when the row is already visible and gives free convergence on the rare chance it isn't, same posture as `process.go`. | +| `Request` not found (`errs.ErrNotFound`) | retryable (`errs.NewRetryableError`) | On a primary-only read this should not happen in practice because `process` commits before it publishes, but the check costs nothing when the row is already visible and gives free convergence on the rare chance it is not, matching `process.go`. | | `Trigger` | raw error; classifier decides | Deliberately left open rather than fixed either way — a runner timeout/connection is transient, a bad URI is permanent, and only a backend classifier can tell them apart. | Everything else — factory lookup, a malformed message, a build-store error other than `ErrAlreadyExists`, and the publish to `buildsignal` — is returned raw with no override, because the default is already correct: none of them are worth automatically replaying (a queue with no registered builder is a config error, a broken payload will never parse, and storage/queue and publish failures dead-letter and let DLQ reconciliation recover). diff --git a/platform/errs/README.md b/platform/errs/README.md index 188406aa..608fac95 100644 --- a/platform/errs/README.md +++ b/platform/errs/README.md @@ -12,7 +12,7 @@ Errors are classified along two axes: | **Infra** | *(any unclassified error)* | `NewRetryableError` | | **Infra dep** | `NewDependencyError` | `NewRetryableDependencyError` | -**Non-retryable by default.** A plain `fmt.Errorf(...)` is treated as a non-retryable infra error. Retryability must be explicitly opted into by wrapping with `NewRetryableError`. This prevents accidental infinite retry loops from unclassified errors. +**Non-retryable by default.** A plain `fmt.Errorf(...)` is treated as a non-retryable infra error. Retryability must be explicitly recognized by a registered classifier or framework wrapper. This prevents accidental infinite retry loops from unclassified errors. **Only infra errors can be retryable.** User errors are never retryable — if a user action caused the failure, retrying the same operation will produce the same result. If an error is retryable, it is by definition an infrastructure issue. @@ -58,7 +58,9 @@ Two implementations ship in this package: ## Adding a Backend-Specific Classifier -Backend classifiers live alongside the extension they classify, under `platform/errs//`. The canonical examples are `platform/errs/mysql` (MySQL driver errors) and `platform/errs/generic` (transport-agnostic concerns such as `context.Canceled`). +Backend classifiers live alongside the extension they classify, under `platform/errs//`. The canonical examples are `platform/errs/mysql` (MySQL driver errors) and `platform/errs/generic` (backend-independent errors such as `context.Canceled` and `errs.ErrVersionMismatch`). + +`platform/errs` also owns shared error identities whose meaning is stable across domains. Identity and classification are separate: `ErrVersionMismatch` is classified as retryable, while `ErrNotFound` remains unclassified and therefore non-retryable by default. A classifier: @@ -110,7 +112,7 @@ Because pass 1 short-circuits on the first framework wrap it finds, **an explici ```go result, err := c.storage.Get(ctx, id) -if errors.Is(err, storage.ErrNotFound) { +if errors.Is(err, errs.ErrNotFound) { // This caller treats "not found" as a user error: the user asked for an // unknown resource. The mysql classifier never gets a vote because the // framework wrap short-circuits pass 1. @@ -131,26 +133,23 @@ Two practical rules fall out of the short-circuit semantics: ### When *not* to classify in a controller -The controller-override path is for the rare case where the controller has certain knowledge a classifier cannot derive from the error value alone — typically a sentinel (`storage.ErrNotFound`) that means "the user asked for something missing" *in this specific call site*. The default and overwhelmingly common case is the opposite: the controller returns the raw error (`return fmt.Errorf("...: %w", err)`) and lets the consumer's `ErrorProcessor` classify it. +The controller-override path is for the rare case where the controller has certain knowledge a classifier cannot derive from the error value alone, such as `errs.ErrNotFound` meaning "the user asked for something missing" in this specific call site. The default and overwhelmingly common case is the opposite: the controller returns the raw error (`return fmt.Errorf("...: %w", err)`) and lets the consumer's `ErrorProcessor` classify it. In particular, **do not reach for `NewRetryableError` just because replaying the message would be convenient.** A failed queue publish, a failed enqueue, a "the hand-off that keeps this alive" step — these are *not* a license to mark the error retryable. Whether such a failure is transient is exactly what a classifier exists to decide: a transport-level classifier wraps genuine connection/timeout blips as retryable, while a malformed-request or permission failure stays non-retryable and dead-letters instead of replaying forever. Blanket `NewRetryableError` on a publish path defeats that and turns every permanent failure into an infinite retry loop. ## Extensions Return Plain Go Errors -Extension interfaces (`MergeChecker`, `Storage`, `Publisher`) return standard `error` values. They may define their own domain-specific sentinel errors (e.g. `storage.ErrNotFound`, `storage.ErrVersionMismatch`) but they do **not** classify errors as user or infra — that is the controller's (and the consumer's `ErrorProcessor`'s) job. +Extension interfaces (`MergeChecker`, `Storage`, `Publisher`) return standard `error` values. They use shared platform errors directly when the meaning is stable across domains, and may define domain-specific sentinels for domain-specific conditions. Extensions do **not** classify errors as user or infra. That is the controller's and the consumer's `ErrorProcessor`'s job. -This separation keeps extensions reusable across contexts. The same `storage.ErrNotFound` might be a user error in one controller (user requested a non-existent resource) and an infra error in another (expected record is missing). +This separation keeps extensions reusable across contexts. The same `errs.ErrNotFound` might be a user error in one controller (the user requested a missing resource) and an infra error in another (an expected record is missing). ## Error Chain Compatibility Framework types preserve the full error chain. Extensions can wrap their own custom errors, and both framework-level and cause-level matching work through `errors.Is`/`errors.As`: ```go -// Extension defines a domain error -var ErrNotFound = errors.New("record not found") - -// Extension implementation wraps it -return fmt.Errorf("request id=%s: %w", id, ErrNotFound) +// Extension implementation wraps a shared error +return fmt.Errorf("request id=%s: %w", id, errs.ErrNotFound) // Controller classifies and wraps again return errs.NewUserError(fmt.Errorf("lookup failed: %w", extensionErr)) @@ -158,7 +157,7 @@ return errs.NewUserError(fmt.Errorf("lookup failed: %w", extensionErr)) // All of these work on the resulting error: errs.IsUserError(err) // true — framework classification errs.IsRetryable(err) // false — user errors are never retryable -errors.Is(err, ErrNotFound) // true — cause is in the chain +errors.Is(err, errs.ErrNotFound) // true: cause is in the chain ``` ## Helpers diff --git a/platform/errs/errs.go b/platform/errs/errs.go index b4c40f98..34e2095e 100644 --- a/platform/errs/errs.go +++ b/platform/errs/errs.go @@ -18,6 +18,13 @@ import ( "errors" ) +// ErrNotFound indicates that a requested resource does not exist. +var ErrNotFound = errors.New("not found") + +// ErrVersionMismatch indicates that an optimistic conditional update failed +// because the persisted version no longer matches the expected version. +var ErrVersionMismatch = errors.New("version mismatch") + // userError represents an error caused by invalid user input or actions. // User errors are never retryable — only infrastructure errors can be retryable. // Use NewUserError to wrap an underlying cause. diff --git a/platform/errs/generic/generic.go b/platform/errs/generic/generic.go index 86ef9bb2..aa2b7f87 100644 --- a/platform/errs/generic/generic.go +++ b/platform/errs/generic/generic.go @@ -39,6 +39,10 @@ type classifier struct{} // must not call errors.Is / errors.As — the classifier-processor owns the // chain walk. func (classifier) Classify(err error) errs.Verdict { + if err == errs.ErrVersionMismatch { + return errs.InfraRetryable + } + // Cancellation signals that the caller aborted the work in flight // (process shutdown, deadline on the inbound RPC, parent operation gone) — // it is not a statement about the work itself being invalid. The same diff --git a/platform/errs/generic/generic_test.go b/platform/errs/generic/generic_test.go index 8656894f..a4681ea8 100644 --- a/platform/errs/generic/generic_test.go +++ b/platform/errs/generic/generic_test.go @@ -24,8 +24,20 @@ import ( "github.com/uber/submitqueue/platform/errs" ) -func TestClassifier_ContextCanceled(t *testing.T) { - assert.Equal(t, errs.InfraRetryable, Classifier.Classify(context.Canceled)) +func TestClassifier_Retryable(t *testing.T) { + tests := []struct { + name string + err error + }{ + {"context canceled", context.Canceled}, + {"version mismatch", errs.ErrVersionMismatch}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, errs.InfraRetryable, Classifier.Classify(tt.err)) + }) + } } func TestClassifier_Unknown(t *testing.T) { @@ -37,6 +49,8 @@ func TestClassifier_Unknown(t *testing.T) { // context.Canceled; the surrounding classifier-processor walk will // reach the inner node and ask Classifier again there. {"wrapped context.Canceled", fmt.Errorf("op: %w", context.Canceled)}, + {"wrapped version mismatch", fmt.Errorf("op: %w", errs.ErrVersionMismatch)}, + {"not found", errs.ErrNotFound}, {"deadline exceeded", context.DeadlineExceeded}, {"plain error", errors.New("anything")}, {"nil", nil}, @@ -65,6 +79,18 @@ func TestClassifier_AppliedViaProcessor(t *testing.T) { assert.True(t, errs.IsRetryable(out)) }) + t.Run("wrapped version mismatch becomes retryable infra", func(t *testing.T) { + wrapped := fmt.Errorf("update: %w", errs.ErrVersionMismatch) + out := p.Process(wrapped) + assert.True(t, errs.IsRetryable(out)) + }) + + t.Run("not found remains non-retryable", func(t *testing.T) { + out := p.Process(fmt.Errorf("get: %w", errs.ErrNotFound)) + assert.False(t, errs.IsRetryable(out)) + assert.ErrorIs(t, out, errs.ErrNotFound) + }) + t.Run("framework wrap in chain wins", func(t *testing.T) { // A controller explicitly classified the shutdown as non-retryable. // The pass-1 framework-wrap check short-circuits before Classifier diff --git a/stovepipe/controller/BUILD.bazel b/stovepipe/controller/BUILD.bazel index da72c78a..5168a97b 100644 --- a/stovepipe/controller/BUILD.bazel +++ b/stovepipe/controller/BUILD.bazel @@ -34,6 +34,7 @@ go_test( deps = [ "//api/stovepipe/protopb:go_default_library", "//platform/consumer:go_default_library", + "//platform/errs:go_default_library", "//platform/extension/counter/mock:go_default_library", "//platform/extension/messagequeue/mock:go_default_library", "//stovepipe/core/messagequeue:go_default_library", diff --git a/stovepipe/controller/dlq/BUILD.bazel b/stovepipe/controller/dlq/BUILD.bazel index 22054560..7c4cbe30 100644 --- a/stovepipe/controller/dlq/BUILD.bazel +++ b/stovepipe/controller/dlq/BUILD.bazel @@ -10,6 +10,7 @@ go_library( visibility = ["//visibility:public"], deps = [ "//platform/consumer:go_default_library", + "//platform/errs:go_default_library", "//platform/metrics:go_default_library", "//stovepipe/core/messagequeue:go_default_library", "//stovepipe/entity:go_default_library", @@ -26,10 +27,10 @@ go_test( deps = [ "//platform/base/messagequeue:go_default_library", "//platform/consumer:go_default_library", + "//platform/errs:go_default_library", "//platform/extension/messagequeue/mock:go_default_library", "//stovepipe/core/messagequeue:go_default_library", "//stovepipe/entity:go_default_library", - "//stovepipe/extension/storage:go_default_library", "//stovepipe/extension/storage/mock:go_default_library", "@com_github_stretchr_testify//assert:go_default_library", "@com_github_stretchr_testify//require:go_default_library", diff --git a/stovepipe/controller/dlq/dlq.go b/stovepipe/controller/dlq/dlq.go index 4912ea3e..099eaf26 100644 --- a/stovepipe/controller/dlq/dlq.go +++ b/stovepipe/controller/dlq/dlq.go @@ -39,6 +39,7 @@ import ( "fmt" "github.com/uber/submitqueue/platform/consumer" + "github.com/uber/submitqueue/platform/errs" "github.com/uber/submitqueue/stovepipe/entity" "github.com/uber/submitqueue/stovepipe/extension/storage" "go.uber.org/zap" @@ -73,7 +74,7 @@ func TopicKey(main consumer.TopicKey) consumer.TopicKey { func failRequest(ctx context.Context, store storage.Storage, logger *zap.SugaredLogger, requestID string) error { request, err := store.GetRequestStore().Get(ctx, requestID) if err != nil { - if errors.Is(err, storage.ErrNotFound) { + if errors.Is(err, errs.ErrNotFound) { logger.Warnw("dlq reconcile: request not found, skipping", "request_id", requestID, ) @@ -117,7 +118,7 @@ func releaseSlot(ctx context.Context, store storage.Storage, logger *zap.Sugared for { queueRow, err := queueStore.Get(ctx, queueName) if err != nil { - if errors.Is(err, storage.ErrNotFound) { + if errors.Is(err, errs.ErrNotFound) { logger.Warnw("dlq reconcile: queue not found, skipping slot release", "queue", queueName, ) @@ -137,7 +138,7 @@ func releaseSlot(ctx context.Context, store storage.Storage, logger *zap.Sugared updated.InFlightCount-- newVersion := queueRow.Version + 1 if err := queueStore.Update(ctx, updated, queueRow.Version, newVersion); err != nil { - if errors.Is(err, storage.ErrVersionMismatch) { + if errors.Is(err, errs.ErrVersionMismatch) { continue } return fmt.Errorf("failed to release slot for queue %s: %w", queueName, err) diff --git a/stovepipe/controller/dlq/dlq_test.go b/stovepipe/controller/dlq/dlq_test.go index 7e6ed037..b3900a75 100644 --- a/stovepipe/controller/dlq/dlq_test.go +++ b/stovepipe/controller/dlq/dlq_test.go @@ -23,10 +23,10 @@ import ( "github.com/uber-go/tally" entityqueue "github.com/uber/submitqueue/platform/base/messagequeue" "github.com/uber/submitqueue/platform/consumer" + "github.com/uber/submitqueue/platform/errs" queuemock "github.com/uber/submitqueue/platform/extension/messagequeue/mock" stovepipemq "github.com/uber/submitqueue/stovepipe/core/messagequeue" "github.com/uber/submitqueue/stovepipe/entity" - "github.com/uber/submitqueue/stovepipe/extension/storage" storagemock "github.com/uber/submitqueue/stovepipe/extension/storage/mock" "go.uber.org/mock/gomock" "go.uber.org/zap" @@ -133,7 +133,7 @@ func TestProcess(t *testing.T) { { name: "request not found is a no-op", setup: func(m dlqMocks) { - m.reqStore.EXPECT().Get(gomock.Any(), testID).Return(entity.Request{}, storage.ErrNotFound) + m.reqStore.EXPECT().Get(gomock.Any(), testID).Return(entity.Request{}, errs.ErrNotFound) }, }, { @@ -142,7 +142,7 @@ func TestProcess(t *testing.T) { m.reqStore.EXPECT().Get(gomock.Any(), testID).Return(requestWithState(entity.RequestStateAccepted), nil) updated := requestWithState(entity.RequestStateAccepted) updated.State = entity.RequestStateRecordedNotGreen - m.reqStore.EXPECT().Update(gomock.Any(), updated, int32(2), int32(3)).Return(storage.ErrVersionMismatch) + m.reqStore.EXPECT().Update(gomock.Any(), updated, int32(2), int32(3)).Return(errs.ErrVersionMismatch) }, wantErr: true, }, @@ -155,7 +155,7 @@ func TestProcess(t *testing.T) { }, nil) m.queueStore.EXPECT().Update(gomock.Any(), entity.Queue{ Name: testQueue, InFlightCount: 0, Version: 5, - }, int32(5), int32(6)).Return(storage.ErrVersionMismatch) + }, int32(5), int32(6)).Return(errs.ErrVersionMismatch) m.queueStore.EXPECT().Get(gomock.Any(), testQueue).Return(entity.Queue{ Name: testQueue, InFlightCount: 1, Version: 6, }, nil) diff --git a/stovepipe/controller/ingest.go b/stovepipe/controller/ingest.go index db97bfd2..fd116780 100644 --- a/stovepipe/controller/ingest.go +++ b/stovepipe/controller/ingest.go @@ -158,7 +158,7 @@ func (c *IngestController) resolveID(ctx context.Context, queue, uri string) (st if id, err := uriStore.GetIDByURI(ctx, queue, uri); err == nil { return id, nil - } else if !errors.Is(err, storage.ErrNotFound) { + } else if !errors.Is(err, errs.ErrNotFound) { return "", fmt.Errorf("IngestController failed to look up existing request for queue=%s: %w", queue, err) } @@ -193,7 +193,7 @@ func (c *IngestController) ensureRequest(ctx context.Context, id, queue, uri str if err == nil { return got, nil } - if !errors.Is(err, storage.ErrNotFound) { + if !errors.Is(err, errs.ErrNotFound) { return entity.Request{}, fmt.Errorf("IngestController failed to load request %s: %w", id, err) } @@ -223,7 +223,7 @@ func (c *IngestController) ensureQueue(ctx context.Context, name string) (entity if err == nil { return got, nil } - if !errors.Is(err, storage.ErrNotFound) { + if !errors.Is(err, errs.ErrNotFound) { return entity.Queue{}, fmt.Errorf("IngestController failed to load queue %s: %w", name, err) } @@ -265,7 +265,7 @@ func (c *IngestController) advanceQueueLatestRequestID(ctx context.Context, queu updated.LatestRequestID = id newVersion := queueRow.Version + 1 if err := queueStore.Update(ctx, updated, queueRow.Version, newVersion); err != nil { - if errors.Is(err, storage.ErrVersionMismatch) { + if errors.Is(err, errs.ErrVersionMismatch) { continue } return fmt.Errorf("IngestController failed to update queue %s latest_request_id: %w", queue, err) diff --git a/stovepipe/controller/ingest_test.go b/stovepipe/controller/ingest_test.go index 18cd66f2..f1774973 100644 --- a/stovepipe/controller/ingest_test.go +++ b/stovepipe/controller/ingest_test.go @@ -23,6 +23,7 @@ import ( "github.com/stretchr/testify/require" "github.com/uber-go/tally" "github.com/uber/submitqueue/platform/consumer" + "github.com/uber/submitqueue/platform/errs" countermock "github.com/uber/submitqueue/platform/extension/counter/mock" mqmock "github.com/uber/submitqueue/platform/extension/messagequeue/mock" stovepipemq "github.com/uber/submitqueue/stovepipe/core/messagequeue" @@ -89,7 +90,7 @@ func expectResolve(m ingestMocks) { // expectAdvanceLatestRequestID wires Get + Create + Update for queue.latest_request_id. func expectAdvanceLatestRequestID(m ingestMocks, queue, id string) { - m.queueStore.EXPECT().Get(gomock.Any(), queue).Return(entity.Queue{}, storage.ErrNotFound) + m.queueStore.EXPECT().Get(gomock.Any(), queue).Return(entity.Queue{}, errs.ErrNotFound) m.queueStore.EXPECT().Create(gomock.Any(), entity.Queue{Name: queue, Version: 1}).Return(nil) updated := entity.Queue{Name: queue, LatestRequestID: id, Version: 1} m.queueStore.EXPECT().Update(gomock.Any(), updated, int32(1), int32(2)).Return(nil) @@ -118,10 +119,10 @@ func TestIngestController_Ingest(t *testing.T) { queue: testQueue, setup: func(m ingestMocks) { expectResolve(m) - m.uriStore.EXPECT().GetIDByURI(gomock.Any(), testQueue, testURI).Return("", storage.ErrNotFound) + m.uriStore.EXPECT().GetIDByURI(gomock.Any(), testQueue, testURI).Return("", errs.ErrNotFound) m.counter.EXPECT().Next(gomock.Any(), "request/"+testQueue).Return(int64(7), nil) m.uriStore.EXPECT().Create(gomock.Any(), testQueue, testURI, "request/monorepo/main/7").Return(nil) - m.reqStore.EXPECT().Get(gomock.Any(), "request/monorepo/main/7").Return(entity.Request{}, storage.ErrNotFound) + m.reqStore.EXPECT().Get(gomock.Any(), "request/monorepo/main/7").Return(entity.Request{}, errs.ErrNotFound) m.reqStore.EXPECT().Create(gomock.Any(), gomock.Any()).Return(nil) expectAdvanceLatestRequestID(m, testQueue, "request/monorepo/main/7") m.publisher.EXPECT().Publish(gomock.Any(), "process", gomock.Any()).Return(nil) @@ -146,7 +147,7 @@ func TestIngestController_Ingest(t *testing.T) { setup: func(m ingestMocks) { expectResolve(m) m.uriStore.EXPECT().GetIDByURI(gomock.Any(), testQueue, testURI).Return("request/monorepo/main/3", nil) - m.reqStore.EXPECT().Get(gomock.Any(), "request/monorepo/main/3").Return(entity.Request{}, storage.ErrNotFound) + m.reqStore.EXPECT().Get(gomock.Any(), "request/monorepo/main/3").Return(entity.Request{}, errs.ErrNotFound) m.reqStore.EXPECT().Create(gomock.Any(), gomock.Any()).Return(nil) expectAdvanceLatestRequestID(m, testQueue, "request/monorepo/main/3") m.publisher.EXPECT().Publish(gomock.Any(), "process", gomock.Any()).Return(nil) @@ -158,7 +159,7 @@ func TestIngestController_Ingest(t *testing.T) { queue: testQueue, setup: func(m ingestMocks) { expectResolve(m) - m.uriStore.EXPECT().GetIDByURI(gomock.Any(), testQueue, testURI).Return("", storage.ErrNotFound) + m.uriStore.EXPECT().GetIDByURI(gomock.Any(), testQueue, testURI).Return("", errs.ErrNotFound) m.counter.EXPECT().Next(gomock.Any(), "request/"+testQueue).Return(int64(7), nil) m.uriStore.EXPECT().Create(gomock.Any(), testQueue, testURI, "request/monorepo/main/7").Return(storage.ErrAlreadyExists) m.uriStore.EXPECT().GetIDByURI(gomock.Any(), testQueue, testURI).Return("request/monorepo/main/3", nil) @@ -199,7 +200,7 @@ func TestIngestController_Ingest(t *testing.T) { queue: testQueue, setup: func(m ingestMocks) { expectResolve(m) - m.uriStore.EXPECT().GetIDByURI(gomock.Any(), testQueue, testURI).Return("", storage.ErrNotFound) + m.uriStore.EXPECT().GetIDByURI(gomock.Any(), testQueue, testURI).Return("", errs.ErrNotFound) m.counter.EXPECT().Next(gomock.Any(), gomock.Any()).Return(int64(0), errors.New("counter unavailable")) }, wantErr: true, @@ -209,10 +210,10 @@ func TestIngestController_Ingest(t *testing.T) { queue: testQueue, setup: func(m ingestMocks) { expectResolve(m) - m.uriStore.EXPECT().GetIDByURI(gomock.Any(), testQueue, testURI).Return("", storage.ErrNotFound) + m.uriStore.EXPECT().GetIDByURI(gomock.Any(), testQueue, testURI).Return("", errs.ErrNotFound) m.counter.EXPECT().Next(gomock.Any(), gomock.Any()).Return(int64(7), nil) m.uriStore.EXPECT().Create(gomock.Any(), testQueue, testURI, gomock.Any()).Return(nil) - m.reqStore.EXPECT().Get(gomock.Any(), gomock.Any()).Return(entity.Request{}, storage.ErrNotFound) + m.reqStore.EXPECT().Get(gomock.Any(), gomock.Any()).Return(entity.Request{}, errs.ErrNotFound) m.reqStore.EXPECT().Create(gomock.Any(), gomock.Any()).Return(errors.New("db down")) }, wantErr: true, @@ -222,10 +223,10 @@ func TestIngestController_Ingest(t *testing.T) { queue: testQueue, setup: func(m ingestMocks) { expectResolve(m) - m.uriStore.EXPECT().GetIDByURI(gomock.Any(), testQueue, testURI).Return("", storage.ErrNotFound) + m.uriStore.EXPECT().GetIDByURI(gomock.Any(), testQueue, testURI).Return("", errs.ErrNotFound) m.counter.EXPECT().Next(gomock.Any(), gomock.Any()).Return(int64(7), nil) m.uriStore.EXPECT().Create(gomock.Any(), testQueue, testURI, gomock.Any()).Return(nil) - m.reqStore.EXPECT().Get(gomock.Any(), gomock.Any()).Return(entity.Request{}, storage.ErrNotFound) + m.reqStore.EXPECT().Get(gomock.Any(), gomock.Any()).Return(entity.Request{}, errs.ErrNotFound) m.reqStore.EXPECT().Create(gomock.Any(), gomock.Any()).Return(nil) expectAdvanceLatestRequestID(m, testQueue, "request/monorepo/main/7") m.publisher.EXPECT().Publish(gomock.Any(), "process", gomock.Any()).Return(errors.New("queue down")) diff --git a/stovepipe/controller/process/BUILD.bazel b/stovepipe/controller/process/BUILD.bazel index 2ff96fdd..e15f130b 100644 --- a/stovepipe/controller/process/BUILD.bazel +++ b/stovepipe/controller/process/BUILD.bazel @@ -34,7 +34,6 @@ go_test( "//stovepipe/extension/queueconfig/default:go_default_library", "//stovepipe/extension/sourcecontrol:go_default_library", "//stovepipe/extension/sourcecontrol/mock:go_default_library", - "//stovepipe/extension/storage:go_default_library", "//stovepipe/extension/storage/mock:go_default_library", "@com_github_stretchr_testify//assert:go_default_library", "@com_github_stretchr_testify//require:go_default_library", diff --git a/stovepipe/controller/process/process.go b/stovepipe/controller/process/process.go index 21b82e83..4eaff1bd 100644 --- a/stovepipe/controller/process/process.go +++ b/stovepipe/controller/process/process.go @@ -215,7 +215,7 @@ func (c *Controller) admitLatestHead(ctx context.Context, request entity.Request if err == nil { break } - if !errors.Is(err, storage.ErrVersionMismatch) { + if !errors.Is(err, errs.ErrVersionMismatch) { return err } // claimBuildSlot reloaded queueRow. Re-coalesce: supersede if a newer head arrived, @@ -298,13 +298,13 @@ func (c *Controller) claimBuildSlot(ctx context.Context, queueRow *entity.Queue) updated.InFlightCount = queueRow.InFlightCount + 1 newVersion := queueRow.Version + 1 if err := queueStore.Update(ctx, updated, queueRow.Version, newVersion); err != nil { - if errors.Is(err, storage.ErrVersionMismatch) { + if errors.Is(err, errs.ErrVersionMismatch) { got, getErr := queueStore.Get(ctx, queueRow.Name) if getErr != nil { return fmt.Errorf("ProcessController failed to reload queue %s after version mismatch: %w", queueRow.Name, getErr) } *queueRow = got - return storage.ErrVersionMismatch + return errs.ErrVersionMismatch } return fmt.Errorf("ProcessController failed to claim build slot for queue %s: %w", queueRow.Name, err) } @@ -332,7 +332,7 @@ func (c *Controller) markProcessing(ctx context.Context, request *entity.Request updated.BaseURI = baseURI newVersion := request.Version + 1 if err := reqStore.Update(ctx, updated, request.Version, newVersion); err != nil { - if errors.Is(err, storage.ErrVersionMismatch) { + if errors.Is(err, errs.ErrVersionMismatch) { got, getErr := reqStore.Get(ctx, request.ID) if getErr != nil { return false, fmt.Errorf("ProcessController failed to reload request %s after version mismatch: %w", request.ID, getErr) @@ -371,7 +371,7 @@ func (c *Controller) releaseBuildSlot(ctx context.Context, queueName string) { updated.InFlightCount = queueRow.InFlightCount - 1 newVersion := queueRow.Version + 1 if err := queueStore.Update(ctx, updated, queueRow.Version, newVersion); err != nil { - if errors.Is(err, storage.ErrVersionMismatch) { + if errors.Is(err, errs.ErrVersionMismatch) { continue } c.logger.Errorw("failed to release claimed build slot", @@ -398,7 +398,7 @@ func (c *Controller) supersedeRequest(ctx context.Context, request entity.Reques updated.State = entity.RequestStateSuperseded newVersion := request.Version + 1 if err := reqStore.Update(ctx, updated, request.Version, newVersion); err != nil { - if errors.Is(err, storage.ErrVersionMismatch) { + if errors.Is(err, errs.ErrVersionMismatch) { got, getErr := reqStore.Get(ctx, request.ID) if getErr != nil { return fmt.Errorf("ProcessController failed to reload request %s after version mismatch: %w", request.ID, getErr) @@ -459,7 +459,7 @@ func (c *Controller) loadRequest(ctx context.Context, id string) (entity.Request if err == nil { return got, nil } - if errors.Is(err, storage.ErrNotFound) { + if errors.Is(err, errs.ErrNotFound) { return entity.Request{}, errs.NewRetryableError(fmt.Errorf("request %s not found yet: %w", id, err)) } return entity.Request{}, fmt.Errorf("ProcessController failed to load request %s: %w", id, err) @@ -471,7 +471,7 @@ func (c *Controller) loadQueue(ctx context.Context, name string) (entity.Queue, if err == nil { return got, nil } - if errors.Is(err, storage.ErrNotFound) { + if errors.Is(err, errs.ErrNotFound) { return entity.Queue{}, errs.NewRetryableError(fmt.Errorf("queue %s not found yet: %w", name, err)) } return entity.Queue{}, fmt.Errorf("ProcessController failed to load queue %s: %w", name, err) diff --git a/stovepipe/controller/process/process_test.go b/stovepipe/controller/process/process_test.go index 80fc1cf8..32ab6264 100644 --- a/stovepipe/controller/process/process_test.go +++ b/stovepipe/controller/process/process_test.go @@ -31,7 +31,6 @@ import ( queueconfigdefault "github.com/uber/submitqueue/stovepipe/extension/queueconfig/default" "github.com/uber/submitqueue/stovepipe/extension/sourcecontrol" sourcecontrolmock "github.com/uber/submitqueue/stovepipe/extension/sourcecontrol/mock" - "github.com/uber/submitqueue/stovepipe/extension/storage" storagemock "github.com/uber/submitqueue/stovepipe/extension/storage/mock" "go.uber.org/mock/gomock" "go.uber.org/zap" @@ -397,7 +396,7 @@ func TestProcessRederivesStrategyAfterQueueReload(t *testing.T) { if tt.initialLastGreen != "" { m.sourceControl.EXPECT().IsAncestor(gomock.Any(), tt.initialLastGreen, testURI).Return(true, nil) } - m.queueStore.EXPECT().Update(gomock.Any(), initialClaim, int32(1), int32(2)).Return(storage.ErrVersionMismatch) + m.queueStore.EXPECT().Update(gomock.Any(), initialClaim, int32(1), int32(2)).Return(errs.ErrVersionMismatch) m.queueStore.EXPECT().Get(gomock.Any(), testQueue).Return(reloadedQueue, nil) m.sourceControl.EXPECT().IsAncestor(gomock.Any(), reloadedLastGreen, testURI).Return(true, nil) m.queueStore.EXPECT().Update(gomock.Any(), claimedQueue, int32(2), int32(3)).Return(nil) @@ -554,7 +553,7 @@ func TestProcess(t *testing.T) { LatestRequestID: testID, InFlightCount: 1, Version: 1, - }, int32(1), int32(2)).Return(storage.ErrVersionMismatch) + }, int32(1), int32(2)).Return(errs.ErrVersionMismatch) m.queueStore.EXPECT().Get(gomock.Any(), testQueue).Return(entity.Queue{ Name: testQueue, LatestRequestID: testID, @@ -576,7 +575,7 @@ func TestProcess(t *testing.T) { // First claim CAS loses to a concurrent writer. m.queueStore.EXPECT().Update(gomock.Any(), entity.Queue{ Name: testQueue, LatestRequestID: testID, InFlightCount: 1, Version: 1, - }, int32(1), int32(2)).Return(storage.ErrVersionMismatch) + }, int32(1), int32(2)).Return(errs.ErrVersionMismatch) // Reload: still latest, slot still free (version advanced by an unrelated field). m.queueStore.EXPECT().Get(gomock.Any(), testQueue).Return(entity.Queue{ Name: testQueue, LatestRequestID: testID, Version: 2, @@ -601,7 +600,7 @@ func TestProcess(t *testing.T) { }, nil) m.queueStore.EXPECT().Update(gomock.Any(), entity.Queue{ Name: testQueue, LatestRequestID: testID, InFlightCount: 1, Version: 1, - }, int32(1), int32(2)).Return(storage.ErrVersionMismatch) + }, int32(1), int32(2)).Return(errs.ErrVersionMismatch) // Reload: ingest stamped a newer head — our head is no longer latest. m.queueStore.EXPECT().Get(gomock.Any(), testQueue).Return(entity.Queue{ Name: testQueue, LatestRequestID: "request/monorepo/main/9", Version: 2, @@ -630,7 +629,7 @@ func TestProcess(t *testing.T) { firstAttempt.State = entity.RequestStateProcessing firstAttempt.BuildStrategy = entity.BuildStrategyIncrementalSinceGreen firstAttempt.BaseURI = lastGreenURI - m.reqStore.EXPECT().Update(gomock.Any(), firstAttempt, int32(1), int32(2)).Return(storage.ErrVersionMismatch) + m.reqStore.EXPECT().Update(gomock.Any(), firstAttempt, int32(1), int32(2)).Return(errs.ErrVersionMismatch) reloaded := acceptedRequest(testID) reloaded.Version = 2 @@ -659,7 +658,7 @@ func TestProcess(t *testing.T) { updatedReq := acceptedRequest(testID) updatedReq.State = entity.RequestStateProcessing updatedReq.BuildStrategy = entity.BuildStrategyFull - m.reqStore.EXPECT().Update(gomock.Any(), updatedReq, int32(1), int32(2)).Return(storage.ErrVersionMismatch) + m.reqStore.EXPECT().Update(gomock.Any(), updatedReq, int32(1), int32(2)).Return(errs.ErrVersionMismatch) m.reqStore.EXPECT().Get(gomock.Any(), testID).Return(entity.Request{ ID: testID, Queue: testQueue, State: entity.RequestStateProcessing, Version: 2, }, nil) @@ -723,7 +722,7 @@ func TestProcess(t *testing.T) { }, nil) updated := acceptedRequest(testOlderID) updated.State = entity.RequestStateSuperseded - m.reqStore.EXPECT().Update(gomock.Any(), updated, int32(1), int32(2)).Return(storage.ErrVersionMismatch) + m.reqStore.EXPECT().Update(gomock.Any(), updated, int32(1), int32(2)).Return(errs.ErrVersionMismatch) m.reqStore.EXPECT().Get(gomock.Any(), testOlderID).Return(entity.Request{ ID: testOlderID, Queue: testQueue, State: entity.RequestStateSuperseded, Version: 2, }, nil) @@ -747,7 +746,7 @@ func TestProcess(t *testing.T) { wantErr: true, wantRetry: true, setup: func(m processMocks) { - m.reqStore.EXPECT().Get(gomock.Any(), testID).Return(entity.Request{}, storage.ErrNotFound) + m.reqStore.EXPECT().Get(gomock.Any(), testID).Return(entity.Request{}, errs.ErrNotFound) }, }, { @@ -756,7 +755,7 @@ func TestProcess(t *testing.T) { wantRetry: true, setup: func(m processMocks) { m.reqStore.EXPECT().Get(gomock.Any(), testID).Return(acceptedRequest(testID), nil) - m.queueStore.EXPECT().Get(gomock.Any(), testQueue).Return(entity.Queue{}, storage.ErrNotFound) + m.queueStore.EXPECT().Get(gomock.Any(), testQueue).Return(entity.Queue{}, errs.ErrNotFound) }, }, { diff --git a/stovepipe/extension/queueconfig/default/BUILD.bazel b/stovepipe/extension/queueconfig/default/BUILD.bazel index f2c8fc3d..a17ab0f4 100644 --- a/stovepipe/extension/queueconfig/default/BUILD.bazel +++ b/stovepipe/extension/queueconfig/default/BUILD.bazel @@ -6,8 +6,8 @@ go_library( importpath = "github.com/uber/submitqueue/stovepipe/extension/queueconfig/default", visibility = ["//visibility:public"], deps = [ + "//platform/errs:go_default_library", "//stovepipe/entity:go_default_library", - "//stovepipe/extension/queueconfig:go_default_library", ], ) @@ -16,7 +16,7 @@ go_test( srcs = ["default_test.go"], embed = [":go_default_library"], deps = [ - "//stovepipe/extension/queueconfig:go_default_library", + "//platform/errs:go_default_library", "@com_github_stretchr_testify//assert:go_default_library", "@com_github_stretchr_testify//require:go_default_library", ], diff --git a/stovepipe/extension/queueconfig/default/default.go b/stovepipe/extension/queueconfig/default/default.go index 5fb69671..0d41f525 100644 --- a/stovepipe/extension/queueconfig/default/default.go +++ b/stovepipe/extension/queueconfig/default/default.go @@ -20,8 +20,8 @@ package defaultconfig import ( "context" + "github.com/uber/submitqueue/platform/errs" "github.com/uber/submitqueue/stovepipe/entity" - "github.com/uber/submitqueue/stovepipe/extension/queueconfig" ) const ( @@ -40,7 +40,7 @@ func NewStore() Store { // Get returns the default configuration for any non-empty queue name. func (Store) Get(_ context.Context, name string) (entity.QueueConfig, error) { if name == "" { - return entity.QueueConfig{}, queueconfig.ErrNotFound + return entity.QueueConfig{}, errs.ErrNotFound } return entity.QueueConfig{ Name: name, diff --git a/stovepipe/extension/queueconfig/default/default_test.go b/stovepipe/extension/queueconfig/default/default_test.go index 490ff35a..8b5900ab 100644 --- a/stovepipe/extension/queueconfig/default/default_test.go +++ b/stovepipe/extension/queueconfig/default/default_test.go @@ -20,7 +20,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - "github.com/uber/submitqueue/stovepipe/extension/queueconfig" + "github.com/uber/submitqueue/platform/errs" ) func TestStore_Get(t *testing.T) { @@ -36,7 +36,7 @@ func TestStore_Get(t *testing.T) { t.Run("empty name is not found", func(t *testing.T) { _, err := store.Get(context.Background(), "") - require.ErrorIs(t, err, queueconfig.ErrNotFound) + require.ErrorIs(t, err, errs.ErrNotFound) }) } diff --git a/stovepipe/extension/queueconfig/queueconfig.go b/stovepipe/extension/queueconfig/queueconfig.go index 437d9a2b..96dcc2f4 100644 --- a/stovepipe/extension/queueconfig/queueconfig.go +++ b/stovepipe/extension/queueconfig/queueconfig.go @@ -18,19 +18,15 @@ package queueconfig import ( "context" - "errors" "github.com/uber/submitqueue/stovepipe/entity" ) -// ErrNotFound is returned when the requested queue configuration does not exist. -var ErrNotFound = errors.New("queue config not found") - // Store loads and provides queue configurations. // Implementations may read from YAML files, databases, remote services, etc. type Store interface { // Get returns the configuration for a named queue. - // Returns ErrNotFound if no configuration exists for the given name. + // Returns the shared platform not-found error if no configuration exists for the given name. Get(ctx context.Context, name string) (entity.QueueConfig, error) // List returns all configured queues. diff --git a/stovepipe/extension/storage/build_store.go b/stovepipe/extension/storage/build_store.go index a6d188d0..4c3f3d66 100644 --- a/stovepipe/extension/storage/build_store.go +++ b/stovepipe/extension/storage/build_store.go @@ -31,11 +31,11 @@ type BuildStore interface { // Returns ErrAlreadyExists if a build with the same ID already exists. Create(ctx context.Context, build entity.Build) error - // Get retrieves a build by ID. Returns ErrNotFound if the build is not found. + // Get retrieves a build by ID. Returns errs.ErrNotFound if the build is not found. Get(ctx context.Context, id string) (entity.Build, error) // Update persists the mutable fields of build if the currently stored version matches - // oldVersion, writing newVersion as the new version. Returns ErrVersionMismatch if the + // oldVersion, writing newVersion as the new version. Returns errs.ErrVersionMismatch if the // stored version does not match (including when the build does not exist). // // Version arithmetic is owned by the caller: it computes newVersion (typically oldVersion+1) diff --git a/stovepipe/extension/storage/mysql/BUILD.bazel b/stovepipe/extension/storage/mysql/BUILD.bazel index 4e9a3612..5694cb08 100644 --- a/stovepipe/extension/storage/mysql/BUILD.bazel +++ b/stovepipe/extension/storage/mysql/BUILD.bazel @@ -12,6 +12,7 @@ go_library( importpath = "github.com/uber/submitqueue/stovepipe/extension/storage/mysql", visibility = ["//visibility:public"], deps = [ + "//platform/errs:go_default_library", "//platform/metrics:go_default_library", "//stovepipe/entity:go_default_library", "//stovepipe/extension/storage:go_default_library", @@ -31,6 +32,7 @@ go_test( ], embed = [":go_default_library"], deps = [ + "//platform/errs:go_default_library", "//stovepipe/entity:go_default_library", "//stovepipe/extension/storage:go_default_library", "@com_github_data_dog_go_sqlmock//:go_default_library", diff --git a/stovepipe/extension/storage/mysql/build_store.go b/stovepipe/extension/storage/mysql/build_store.go index 45dd5aef..6616ad9b 100644 --- a/stovepipe/extension/storage/mysql/build_store.go +++ b/stovepipe/extension/storage/mysql/build_store.go @@ -22,6 +22,7 @@ import ( "github.com/uber-go/tally" + "github.com/uber/submitqueue/platform/errs" "github.com/uber/submitqueue/platform/metrics" "github.com/uber/submitqueue/stovepipe/entity" "github.com/uber/submitqueue/stovepipe/extension/storage" @@ -60,7 +61,7 @@ func (b *buildStore) Create(ctx context.Context, build entity.Build) (retErr err return nil } -// Get retrieves a build by ID. Returns ErrNotFound if the build is not found. +// Get retrieves a build by ID. Returns errs.ErrNotFound if the build is not found. func (b *buildStore) Get(ctx context.Context, id string) (ret entity.Build, retErr error) { op := metrics.Begin(b.scope, "get") defer func() { op.Complete(retErr) }() @@ -78,7 +79,7 @@ func (b *buildStore) Get(ctx context.Context, id string) (ret entity.Build, retE ) if errors.Is(err, sql.ErrNoRows) { - return entity.Build{}, storage.WrapNotFound(err) + return entity.Build{}, fmt.Errorf("%w: %w", errs.ErrNotFound, err) } if err != nil { return entity.Build{}, fmt.Errorf("failed to get build entity id=%s from the database: %w", id, err) @@ -88,7 +89,7 @@ func (b *buildStore) Get(ctx context.Context, id string) (ret entity.Build, retE } // Update persists the mutable fields of build (status) if the stored version matches -// oldVersion, writing newVersion. Returns ErrVersionMismatch if the stored version does not +// oldVersion, writing newVersion. Returns errs.ErrVersionMismatch if the stored version does not // match (including when the build does not exist). This is a pure conditional write; the // caller owns version arithmetic. func (b *buildStore) Update(ctx context.Context, build entity.Build, oldVersion, newVersion int32) (retErr error) { @@ -122,7 +123,7 @@ func (b *buildStore) Update(ctx context.Context, build entity.Build, oldVersion, if rowsAffected != 1 { return fmt.Errorf( "version mismatch for build update: id=%q expected_version=%d: %w", - build.ID, oldVersion, storage.ErrVersionMismatch, + build.ID, oldVersion, errs.ErrVersionMismatch, ) } diff --git a/stovepipe/extension/storage/mysql/build_store_test.go b/stovepipe/extension/storage/mysql/build_store_test.go index f9425986..7ffa4560 100644 --- a/stovepipe/extension/storage/mysql/build_store_test.go +++ b/stovepipe/extension/storage/mysql/build_store_test.go @@ -25,6 +25,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/uber/submitqueue/platform/errs" "github.com/uber/submitqueue/stovepipe/entity" "github.com/uber/submitqueue/stovepipe/extension/storage" ) @@ -141,7 +142,7 @@ func TestBuildStore_Get(t *testing.T) { WillReturnError(sql.ErrNoRows) }, wantErr: true, - wantErrIs: storage.ErrNotFound, + wantErrIs: errs.ErrNotFound, }, { name: "query error", @@ -203,7 +204,7 @@ func TestBuildStore_Update(t *testing.T) { WillReturnResult(sqlmock.NewResult(0, 0)) }, wantErr: true, - wantErrIs: storage.ErrVersionMismatch, + wantErrIs: errs.ErrVersionMismatch, }, { name: "exec error", diff --git a/stovepipe/extension/storage/mysql/queue_store.go b/stovepipe/extension/storage/mysql/queue_store.go index ee922d31..b02efba2 100644 --- a/stovepipe/extension/storage/mysql/queue_store.go +++ b/stovepipe/extension/storage/mysql/queue_store.go @@ -21,6 +21,7 @@ import ( "fmt" "github.com/uber-go/tally" + "github.com/uber/submitqueue/platform/errs" "github.com/uber/submitqueue/platform/metrics" "github.com/uber/submitqueue/stovepipe/entity" "github.com/uber/submitqueue/stovepipe/extension/storage" @@ -59,7 +60,7 @@ func (q *queueStore) Create(ctx context.Context, queue entity.Queue) (retErr err return nil } -// Get retrieves a queue by name. Returns ErrNotFound if the queue is not found. +// Get retrieves a queue by name. Returns errs.ErrNotFound if the queue is not found. func (q *queueStore) Get(ctx context.Context, name string) (ret entity.Queue, retErr error) { op := metrics.Begin(q.scope, "get") defer func() { op.Complete(retErr) }() @@ -77,7 +78,7 @@ func (q *queueStore) Get(ctx context.Context, name string) (ret entity.Queue, re ) if errors.Is(err, sql.ErrNoRows) { - return entity.Queue{}, storage.WrapNotFound(err) + return entity.Queue{}, fmt.Errorf("%w: %w", errs.ErrNotFound, err) } if err != nil { return entity.Queue{}, fmt.Errorf("failed to get queue name=%s from the database: %w", name, err) @@ -87,7 +88,7 @@ func (q *queueStore) Get(ctx context.Context, name string) (ret entity.Queue, re } // Update persists the mutable fields of queue if the stored version matches oldVersion, -// writing newVersion. Returns ErrVersionMismatch if the stored version does not match. +// writing newVersion. Returns errs.ErrVersionMismatch if the stored version does not match. func (q *queueStore) Update(ctx context.Context, queue entity.Queue, oldVersion, newVersion int32) (retErr error) { op := metrics.Begin(q.scope, "update") defer func() { op.Complete(retErr) }() @@ -121,7 +122,7 @@ func (q *queueStore) Update(ctx context.Context, queue entity.Queue, oldVersion, if rowsAffected != 1 { return fmt.Errorf( "version mismatch for queue update: name=%q expected_version=%d: %w", - queue.Name, oldVersion, storage.ErrVersionMismatch, + queue.Name, oldVersion, errs.ErrVersionMismatch, ) } diff --git a/stovepipe/extension/storage/mysql/queue_store_test.go b/stovepipe/extension/storage/mysql/queue_store_test.go index b766d2ab..165b00a8 100644 --- a/stovepipe/extension/storage/mysql/queue_store_test.go +++ b/stovepipe/extension/storage/mysql/queue_store_test.go @@ -25,6 +25,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/uber/submitqueue/platform/errs" "github.com/uber/submitqueue/stovepipe/entity" "github.com/uber/submitqueue/stovepipe/extension/storage" ) @@ -143,7 +144,7 @@ func TestQueueStore_Get(t *testing.T) { WillReturnError(sql.ErrNoRows) }, wantErr: true, - wantErrIs: storage.ErrNotFound, + wantErrIs: errs.ErrNotFound, }, { name: "query error", @@ -210,7 +211,7 @@ func TestQueueStore_Update(t *testing.T) { WillReturnResult(sqlmock.NewResult(0, 0)) }, wantErr: true, - wantErrIs: storage.ErrVersionMismatch, + wantErrIs: errs.ErrVersionMismatch, }, { name: "exec error", diff --git a/stovepipe/extension/storage/mysql/request_store.go b/stovepipe/extension/storage/mysql/request_store.go index 7e7d112e..cce85283 100644 --- a/stovepipe/extension/storage/mysql/request_store.go +++ b/stovepipe/extension/storage/mysql/request_store.go @@ -23,6 +23,7 @@ import ( "github.com/go-sql-driver/mysql" "github.com/uber-go/tally" + "github.com/uber/submitqueue/platform/errs" "github.com/uber/submitqueue/platform/metrics" "github.com/uber/submitqueue/stovepipe/entity" "github.com/uber/submitqueue/stovepipe/extension/storage" @@ -68,7 +69,7 @@ func (r *requestStore) Create(ctx context.Context, request entity.Request) (retE return nil } -// Get retrieves a request by ID. Returns ErrNotFound if the request is not found. +// Get retrieves a request by ID. Returns errs.ErrNotFound if the request is not found. func (r *requestStore) Get(ctx context.Context, id string) (ret entity.Request, retErr error) { op := metrics.Begin(r.scope, "get") defer func() { op.Complete(retErr) }() @@ -89,7 +90,7 @@ func (r *requestStore) Get(ctx context.Context, id string) (ret entity.Request, ) if errors.Is(err, sql.ErrNoRows) { - return entity.Request{}, storage.WrapNotFound(err) + return entity.Request{}, fmt.Errorf("%w: %w", errs.ErrNotFound, err) } if err != nil { return entity.Request{}, fmt.Errorf("failed to get request entity id=%s from the database: %w", id, err) @@ -99,7 +100,7 @@ func (r *requestStore) Get(ctx context.Context, id string) (ret entity.Request, } // Update persists the mutable fields of request (uri, state, build_strategy, base_uri) if the -// oldVersion, writing newVersion. Returns ErrVersionMismatch if the stored version does not match +// oldVersion, writing newVersion. Returns errs.ErrVersionMismatch if the stored version does not match // (including when the request does not exist). This is a pure conditional write; the caller owns // version arithmetic. func (r *requestStore) Update(ctx context.Context, request entity.Request, oldVersion, newVersion int32) (retErr error) { @@ -136,7 +137,7 @@ func (r *requestStore) Update(ctx context.Context, request entity.Request, oldVe if rowsAffected != 1 { return fmt.Errorf( "version mismatch for request update: id=%q expected_version=%d: %w", - request.ID, oldVersion, storage.ErrVersionMismatch, + request.ID, oldVersion, errs.ErrVersionMismatch, ) } diff --git a/stovepipe/extension/storage/mysql/request_store_test.go b/stovepipe/extension/storage/mysql/request_store_test.go index b12465ce..1c6033cf 100644 --- a/stovepipe/extension/storage/mysql/request_store_test.go +++ b/stovepipe/extension/storage/mysql/request_store_test.go @@ -26,6 +26,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/uber/submitqueue/platform/errs" "github.com/uber/submitqueue/stovepipe/entity" "github.com/uber/submitqueue/stovepipe/extension/storage" ) @@ -148,7 +149,7 @@ func TestRequestStore_Get(t *testing.T) { WillReturnError(sql.ErrNoRows) }, wantErr: true, - wantErrIs: storage.ErrNotFound, + wantErrIs: errs.ErrNotFound, }, { name: "query error", @@ -216,7 +217,7 @@ func TestRequestStore_Update(t *testing.T) { WillReturnResult(sqlmock.NewResult(0, 0)) }, wantErr: true, - wantErrIs: storage.ErrVersionMismatch, + wantErrIs: errs.ErrVersionMismatch, }, { name: "exec error", diff --git a/stovepipe/extension/storage/mysql/request_uri_store.go b/stovepipe/extension/storage/mysql/request_uri_store.go index 1f5c5aea..3dfe844c 100644 --- a/stovepipe/extension/storage/mysql/request_uri_store.go +++ b/stovepipe/extension/storage/mysql/request_uri_store.go @@ -22,6 +22,7 @@ import ( "github.com/uber-go/tally" + "github.com/uber/submitqueue/platform/errs" "github.com/uber/submitqueue/platform/metrics" "github.com/uber/submitqueue/stovepipe/extension/storage" ) @@ -61,7 +62,7 @@ func (r *requestURIStore) Create(ctx context.Context, queue, uri, id string) (re return nil } -// GetIDByURI returns the id of the request validating (queue, uri). Returns ErrNotFound if absent. +// GetIDByURI returns the id of the request validating (queue, uri). Returns errs.ErrNotFound if absent. func (r *requestURIStore) GetIDByURI(ctx context.Context, queue, uri string) (ret string, retErr error) { op := metrics.Begin(r.scope, "get_id_by_uri") defer func() { op.Complete(retErr) }() @@ -73,7 +74,7 @@ func (r *requestURIStore) GetIDByURI(ctx context.Context, queue, uri string) (re ).Scan(&id) if errors.Is(err, sql.ErrNoRows) { - return "", storage.WrapNotFound(err) + return "", fmt.Errorf("%w: %w", errs.ErrNotFound, err) } if err != nil { return "", fmt.Errorf("failed to get request id for queue=%s uri=%s from the database: %w", queue, uri, err) diff --git a/stovepipe/extension/storage/mysql/request_uri_store_test.go b/stovepipe/extension/storage/mysql/request_uri_store_test.go index 1e7b3765..e372d16a 100644 --- a/stovepipe/extension/storage/mysql/request_uri_store_test.go +++ b/stovepipe/extension/storage/mysql/request_uri_store_test.go @@ -25,6 +25,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/uber/submitqueue/platform/errs" "github.com/uber/submitqueue/stovepipe/extension/storage" ) @@ -132,7 +133,7 @@ func TestRequestURIStore_GetIDByURI(t *testing.T) { WillReturnError(sql.ErrNoRows) }, wantErr: true, - wantErrIs: storage.ErrNotFound, + wantErrIs: errs.ErrNotFound, }, { name: "query error", diff --git a/stovepipe/extension/storage/queue_store.go b/stovepipe/extension/storage/queue_store.go index b47f34b9..a441e3a2 100644 --- a/stovepipe/extension/storage/queue_store.go +++ b/stovepipe/extension/storage/queue_store.go @@ -28,11 +28,11 @@ type QueueStore interface { // if a row with the same name already exists. Create(ctx context.Context, queue entity.Queue) error - // Get retrieves a queue by name. Returns ErrNotFound if the queue is not found. + // Get retrieves a queue by name. Returns errs.ErrNotFound if the queue is not found. Get(ctx context.Context, name string) (entity.Queue, error) // Update persists the mutable fields of queue if the stored version matches - // oldVersion, writing newVersion. Returns ErrVersionMismatch if the stored + // oldVersion, writing newVersion. Returns errs.ErrVersionMismatch if the stored // version does not match (including when the queue does not exist). // // Version arithmetic is owned by the caller: it computes newVersion (typically diff --git a/stovepipe/extension/storage/request_store.go b/stovepipe/extension/storage/request_store.go index 4001bf0f..da54f838 100644 --- a/stovepipe/extension/storage/request_store.go +++ b/stovepipe/extension/storage/request_store.go @@ -30,11 +30,11 @@ type RequestStore interface { // Returns ErrAlreadyExists if a request with the same ID already exists. Create(ctx context.Context, request entity.Request) error - // Get retrieves a request by ID. Returns ErrNotFound if the request is not found. + // Get retrieves a request by ID. Returns errs.ErrNotFound if the request is not found. Get(ctx context.Context, id string) (entity.Request, error) // Update persists the mutable fields of request if the currently stored version matches - // oldVersion, writing newVersion as the new version. Returns ErrVersionMismatch if the + // oldVersion, writing newVersion as the new version. Returns errs.ErrVersionMismatch if the // stored version does not match (including when the request does not exist). // // Version arithmetic is owned by the caller: it computes newVersion (typically oldVersion+1) diff --git a/stovepipe/extension/storage/request_uri_store.go b/stovepipe/extension/storage/request_uri_store.go index e5f9f6b7..3fe81034 100644 --- a/stovepipe/extension/storage/request_uri_store.go +++ b/stovepipe/extension/storage/request_uri_store.go @@ -31,6 +31,6 @@ type RequestURIStore interface { Create(ctx context.Context, queue, uri, id string) error // GetIDByURI returns the id of the request validating (queue, uri). - // Returns ErrNotFound if no request is mapped to that commit. + // Returns errs.ErrNotFound if no request is mapped to that commit. GetIDByURI(ctx context.Context, queue, uri string) (string, error) } diff --git a/stovepipe/extension/storage/storage.go b/stovepipe/extension/storage/storage.go index 46eee6c7..5310ac5f 100644 --- a/stovepipe/extension/storage/storage.go +++ b/stovepipe/extension/storage/storage.go @@ -18,30 +18,11 @@ package storage import ( "errors" - "fmt" ) -// ErrNotFound is returned by storage implementations when the requested record is not found in the database. -var ErrNotFound = errors.New("record not found") - -// IsNotFound returns true if any error in the error chain is a ErrNotFound. -func IsNotFound(err error) bool { - return errors.Is(err, ErrNotFound) -} - -// WrapNotFound wraps ErrNotFound with the original error from the storage implementation. -func WrapNotFound(err error) error { - return fmt.Errorf("%w: %w", ErrNotFound, err) -} - // ErrAlreadyExists is returned by storage implementations when attempting to create a record that already exists. var ErrAlreadyExists = errors.New("record already exists") -// ErrVersionMismatch is returned by storage implementations when a conditional (CAS) update finds that -// the stored version does not match the expected version. It backs optimistic locking, letting callers -// retry or converge instead of overwriting a concurrent change. -var ErrVersionMismatch = errors.New("version mismatch") - // Storage is a factory interface that aggregates all entity stores into a single injectable dependency. type Storage interface { // GetRequestStore returns the RequestStore instance. diff --git a/submitqueue/core/request/BUILD.bazel b/submitqueue/core/request/BUILD.bazel index 63c149bc..d8db3a90 100644 --- a/submitqueue/core/request/BUILD.bazel +++ b/submitqueue/core/request/BUILD.bazel @@ -12,6 +12,7 @@ go_library( deps = [ "//platform/base/messagequeue:go_default_library", "//platform/consumer:go_default_library", + "//platform/errs:go_default_library", "//submitqueue/core/topickey:go_default_library", "//submitqueue/entity:go_default_library", "//submitqueue/extension/storage:go_default_library", @@ -29,10 +30,10 @@ go_test( deps = [ "//platform/base/messagequeue:go_default_library", "//platform/consumer:go_default_library", + "//platform/errs:go_default_library", "//platform/extension/messagequeue/mock:go_default_library", "//submitqueue/core/topickey:go_default_library", "//submitqueue/entity:go_default_library", - "//submitqueue/extension/storage:go_default_library", "//submitqueue/extension/storage/mock:go_default_library", "@com_github_stretchr_testify//assert:go_default_library", "@com_github_stretchr_testify//require:go_default_library", diff --git a/submitqueue/core/request/materializer.go b/submitqueue/core/request/materializer.go index 909d1a9b..746a3e39 100644 --- a/submitqueue/core/request/materializer.go +++ b/submitqueue/core/request/materializer.go @@ -21,6 +21,7 @@ import ( "maps" "slices" + "github.com/uber/submitqueue/platform/errs" "github.com/uber/submitqueue/submitqueue/entity" "github.com/uber/submitqueue/submitqueue/extension/storage" ) @@ -61,7 +62,7 @@ func (m *Materializer) PersistLog(ctx context.Context, log entity.RequestLog) er updated.Metadata = cloneMetadata(log.Metadata) if err := m.store.GetRequestSummaryStore().Update(ctx, updated, oldVersion, newVersion); err != nil { - if errors.Is(err, storage.ErrVersionMismatch) { + if errors.Is(err, errs.ErrVersionMismatch) { continue } return fmt.Errorf("failed to update request summary request_id=%s: %w", log.RequestID, err) @@ -83,7 +84,7 @@ func (m *Materializer) repairPublicProjections(ctx context.Context, authoritativ desired := queueSummaryFromSummary(authoritative) for { current, err := m.store.GetRequestQueueSummaryStore().Get(ctx, desired.Queue, desired.ReceivedAtMs, desired.RequestID) - if errors.Is(err, storage.ErrNotFound) { + if errors.Is(err, errs.ErrNotFound) { if err := m.createURIMappings(ctx, authoritative); err != nil { return err } @@ -106,7 +107,7 @@ func (m *Materializer) repairPublicProjections(ctx context.Context, authoritativ return nil } if err := m.store.GetRequestQueueSummaryStore().Update(ctx, desired, current.Version, desired.Version); err != nil { - if errors.Is(err, storage.ErrVersionMismatch) { + if errors.Is(err, errs.ErrVersionMismatch) { continue } return fmt.Errorf("failed to update queue summary request_id=%s: %w", desired.RequestID, err) diff --git a/submitqueue/core/request/materializer_test.go b/submitqueue/core/request/materializer_test.go index fd6c815c..34ff6b44 100644 --- a/submitqueue/core/request/materializer_test.go +++ b/submitqueue/core/request/materializer_test.go @@ -21,8 +21,8 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/uber/submitqueue/platform/errs" "github.com/uber/submitqueue/submitqueue/entity" - "github.com/uber/submitqueue/submitqueue/extension/storage" storagemock "github.com/uber/submitqueue/submitqueue/extension/storage/mock" "go.uber.org/mock/gomock" ) @@ -68,7 +68,7 @@ func TestMaterializer_PersistLog(t *testing.T) { store, summaryStore, queueStore, _, logStore := materializerStores(ctrl) logStore.EXPECT().Insert(gomock.Any(), log).Return(nil) summaryStore.EXPECT().Get(gomock.Any(), "q/1").Return(base, nil) - summaryStore.EXPECT().Update(gomock.Any(), gomock.Any(), int32(1), int32(2)).Return(storage.ErrVersionMismatch) + summaryStore.EXPECT().Update(gomock.Any(), gomock.Any(), int32(1), int32(2)).Return(errs.ErrVersionMismatch) advanced := base advanced.Status = entity.RequestStatusLanded advanced.RequestVersion = 2 @@ -106,7 +106,7 @@ func TestMaterializer_PersistLog(t *testing.T) { activated.RequestVersion = 2 activated.StatusTimestampMs = 20 activated.Version = 2 - queueStore.EXPECT().Get(gomock.Any(), "q", int64(10), "q/1").Return(entity.RequestQueueSummary{}, storage.ErrNotFound) + queueStore.EXPECT().Get(gomock.Any(), "q", int64(10), "q/1").Return(entity.RequestQueueSummary{}, errs.ErrNotFound) uriStore.EXPECT().Create(gomock.Any(), entity.RequestURI{ChangeURI: "uri/1", ReceivedAtMs: 10, RequestID: "q/1"}).Return(nil) uriStore.EXPECT().Create(gomock.Any(), entity.RequestURI{ChangeURI: "uri/2", ReceivedAtMs: 10, RequestID: "q/1"}).Return(nil) queueStore.EXPECT().Create(gomock.Any(), queueSummaryFromSummary(activated)).Return(nil) @@ -135,7 +135,7 @@ func TestMaterializer_PersistLog(t *testing.T) { ctrl := gomock.NewController(t) store, summaryStore, _, _, logStore := materializerStores(ctrl) logStore.EXPECT().Insert(gomock.Any(), log).Return(nil) - summaryStore.EXPECT().Get(gomock.Any(), "q/1").Return(entity.RequestSummary{}, storage.ErrNotFound) + summaryStore.EXPECT().Get(gomock.Any(), "q/1").Return(entity.RequestSummary{}, errs.ErrNotFound) require.Error(t, NewMaterializer(store).PersistLog(context.Background(), log)) }) diff --git a/submitqueue/core/request/request_test.go b/submitqueue/core/request/request_test.go index 75a955f9..96a6809c 100644 --- a/submitqueue/core/request/request_test.go +++ b/submitqueue/core/request/request_test.go @@ -23,8 +23,8 @@ import ( "github.com/stretchr/testify/require" "go.uber.org/mock/gomock" + "github.com/uber/submitqueue/platform/errs" "github.com/uber/submitqueue/submitqueue/entity" - "github.com/uber/submitqueue/submitqueue/extension/storage" storagemock "github.com/uber/submitqueue/submitqueue/extension/storage/mock" ) @@ -110,11 +110,11 @@ func TestGetCurrentStateFromRequestLog(t *testing.T) { func TestGetCurrentStateFromRequestLog_NoRecords(t *testing.T) { ctrl := gomock.NewController(t) mockStore := storagemock.NewMockRequestLogStore(ctrl) - mockStore.EXPECT().List(gomock.Any(), "q/1").Return(nil, storage.ErrNotFound) + mockStore.EXPECT().List(gomock.Any(), "q/1").Return(nil, errs.ErrNotFound) _, err := GetCurrentStateFromRequestLog(context.Background(), mockStore, "q/1") assert.Error(t, err) - assert.True(t, storage.IsNotFound(err)) + assert.ErrorIs(t, err, errs.ErrNotFound) } func TestGetCurrentStateFromRequestLog_StoreError(t *testing.T) { diff --git a/submitqueue/extension/queueconfig/queueconfig.go b/submitqueue/extension/queueconfig/queueconfig.go index 0161ed1f..19561f5f 100644 --- a/submitqueue/extension/queueconfig/queueconfig.go +++ b/submitqueue/extension/queueconfig/queueconfig.go @@ -18,19 +18,15 @@ package queueconfig import ( "context" - "errors" "github.com/uber/submitqueue/submitqueue/entity" ) -// ErrNotFound is returned when the requested queue configuration does not exist. -var ErrNotFound = errors.New("queue config not found") - // Store loads and provides queue configurations. // Implementations may read from YAML files, databases, remote services, etc. type Store interface { // Get returns the configuration for a named queue. - // Returns ErrNotFound if no configuration exists for the given name. + // Returns the shared platform not-found error if no configuration exists for the given name. Get(ctx context.Context, name string) (entity.QueueConfig, error) // List returns all configured queues. diff --git a/submitqueue/extension/queueconfig/yaml/BUILD.bazel b/submitqueue/extension/queueconfig/yaml/BUILD.bazel index d11e0d2f..916a1890 100644 --- a/submitqueue/extension/queueconfig/yaml/BUILD.bazel +++ b/submitqueue/extension/queueconfig/yaml/BUILD.bazel @@ -6,8 +6,8 @@ go_library( importpath = "github.com/uber/submitqueue/submitqueue/extension/queueconfig/yaml", visibility = ["//visibility:public"], deps = [ + "//platform/errs:go_default_library", "//submitqueue/entity:go_default_library", - "//submitqueue/extension/queueconfig:go_default_library", "@in_gopkg_yaml_v3//:go_default_library", ], ) @@ -17,7 +17,7 @@ go_test( srcs = ["yaml_test.go"], embed = [":go_default_library"], deps = [ - "//submitqueue/extension/queueconfig:go_default_library", + "//platform/errs:go_default_library", "@com_github_stretchr_testify//assert:go_default_library", "@com_github_stretchr_testify//require:go_default_library", ], diff --git a/submitqueue/extension/queueconfig/yaml/yaml.go b/submitqueue/extension/queueconfig/yaml/yaml.go index f8afa48d..9b9d575f 100644 --- a/submitqueue/extension/queueconfig/yaml/yaml.go +++ b/submitqueue/extension/queueconfig/yaml/yaml.go @@ -22,8 +22,8 @@ import ( "fmt" "os" + "github.com/uber/submitqueue/platform/errs" "github.com/uber/submitqueue/submitqueue/entity" - "github.com/uber/submitqueue/submitqueue/extension/queueconfig" yamlv3 "gopkg.in/yaml.v3" ) @@ -73,11 +73,11 @@ func NewStore(path string) (Store, error) { return Store{byName: byName, all: all}, nil } -// Get returns the configuration for the named queue, or queueconfig.ErrNotFound. +// Get returns the configuration for the named queue, or errs.ErrNotFound. func (s Store) Get(_ context.Context, name string) (entity.QueueConfig, error) { cfg, ok := s.byName[name] if !ok { - return entity.QueueConfig{}, queueconfig.ErrNotFound + return entity.QueueConfig{}, errs.ErrNotFound } return cfg, nil } diff --git a/submitqueue/extension/queueconfig/yaml/yaml_test.go b/submitqueue/extension/queueconfig/yaml/yaml_test.go index cd005e8e..432152cf 100644 --- a/submitqueue/extension/queueconfig/yaml/yaml_test.go +++ b/submitqueue/extension/queueconfig/yaml/yaml_test.go @@ -23,7 +23,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - "github.com/uber/submitqueue/submitqueue/extension/queueconfig" + "github.com/uber/submitqueue/platform/errs" ) func writeTempYAML(t *testing.T, contents string) string { @@ -136,7 +136,7 @@ func TestStore_Get(t *testing.T) { t.Run("unknown queue returns ErrNotFound", func(t *testing.T) { _, err := store.Get(context.Background(), "nope") require.Error(t, err) - assert.True(t, errors.Is(err, queueconfig.ErrNotFound)) + assert.True(t, errors.Is(err, errs.ErrNotFound)) }) } diff --git a/submitqueue/extension/storage/README.md b/submitqueue/extension/storage/README.md index c82e875b..d323d19b 100644 --- a/submitqueue/extension/storage/README.md +++ b/submitqueue/extension/storage/README.md @@ -4,7 +4,7 @@ Pluggable persistence interfaces for SubmitQueue entities (requests, batches, de ## Optimistic locking contract -Entities that support concurrent mutation carry an `int32 Version` field. Updates are conditional on the version: the write only succeeds if the persisted version matches the caller's expected version. On mismatch, the implementation returns `storage.ErrVersionMismatch`. +Entities that support concurrent mutation carry an `int32 Version` field. Updates are conditional on the version: the write only succeeds if the persisted version matches the caller's expected version. On mismatch, the implementation returns `errs.ErrVersionMismatch`. **Version arithmetic is owned by the controller, not the store.** Update methods take both `oldVersion` (the where-clause guard) and `newVersion` (the value to write): diff --git a/submitqueue/extension/storage/batch_dependent_store.go b/submitqueue/extension/storage/batch_dependent_store.go index 595afedc..f4cff681 100644 --- a/submitqueue/extension/storage/batch_dependent_store.go +++ b/submitqueue/extension/storage/batch_dependent_store.go @@ -28,12 +28,12 @@ import ( // The batch-creation flow always calls Create here before creating the Batch itself, so every active // Batch is guaranteed to have a corresponding BatchDependent row. Lookups via Get are only performed // for batch IDs returned from the active-batch set, meaning a missing row indicates data corruption or -// out-of-band manipulation rather than a normal "not found" outcome. ErrNotFound is therefore part of +// out-of-band manipulation rather than a normal "not found" outcome. errs.ErrNotFound is therefore part of // the contract for completeness but is not expected to be returned in steady-state operation. type BatchDependentStore interface { // Get retrieves the batch dependent by batch ID. // If the batch contains no dependents, the returned BatchDependent will have an empty Dependents list. - // Returns ErrNotFound if the batch itself is not found, which should never happen in steady-state system and + // Returns errs.ErrNotFound if the batch itself is not found, which should never happen in steady-state system and // therefore does not need a special handling. Get(ctx context.Context, batchID string) (entity.BatchDependent, error) @@ -42,7 +42,7 @@ type BatchDependentStore interface { Create(ctx context.Context, batchDependent entity.BatchDependent) error // UpdateDependents updates the dependents of a batch dependent and the version to newVersion - // if the current persisted version matches oldVersion. If versions do not match, returns ErrVersionMismatch. + // if the current persisted version matches oldVersion. If versions do not match, returns errs.ErrVersionMismatch. // Version arithmetic is owned by the caller; the store performs a pure conditional write. UpdateDependents(ctx context.Context, batchID string, oldVersion, newVersion int32, dependents []string) error } diff --git a/submitqueue/extension/storage/batch_store.go b/submitqueue/extension/storage/batch_store.go index 05e94fb8..bb977ac6 100644 --- a/submitqueue/extension/storage/batch_store.go +++ b/submitqueue/extension/storage/batch_store.go @@ -24,7 +24,7 @@ import ( // BatchStore is an interface that defines methods for managing batches in the database. type BatchStore interface { - // Get retrieves a batch by ID. Returns ErrNotFound if the batch is not found. + // Get retrieves a batch by ID. Returns errs.ErrNotFound if the batch is not found. Get(ctx context.Context, id string) (entity.Batch, error) // Create creates a new batch. The batch must have a unique ID already assigned. @@ -32,12 +32,12 @@ type BatchStore interface { Create(ctx context.Context, batch entity.Batch) error // UpdateState updates the state of a batch to newState and the version to newVersion - // if the current persisted version matches oldVersion. If versions do not match, returns ErrVersionMismatch. + // if the current persisted version matches oldVersion. If versions do not match, returns errs.ErrVersionMismatch. // Version arithmetic is owned by the caller; the store performs a pure conditional write. UpdateState(ctx context.Context, id string, oldVersion, newVersion int32, newState entity.BatchState) error // UpdateScoreAndState atomically updates the score and state of a batch and the version to newVersion - // if the current persisted version matches oldVersion. If versions do not match, returns ErrVersionMismatch. + // if the current persisted version matches oldVersion. If versions do not match, returns errs.ErrVersionMismatch. // Version arithmetic is owned by the caller; the store performs a pure conditional write. UpdateScoreAndState(ctx context.Context, id string, oldVersion, newVersion int32, score float64, newState entity.BatchState) error diff --git a/submitqueue/extension/storage/build_store.go b/submitqueue/extension/storage/build_store.go index a2f89bc4..0b35a35c 100644 --- a/submitqueue/extension/storage/build_store.go +++ b/submitqueue/extension/storage/build_store.go @@ -24,7 +24,7 @@ import ( // BuildStore is an interface that defines methods for managing builds in the database. type BuildStore interface { - // Get retrieves a build by ID. Returns ErrNotFound if the build is not found. + // Get retrieves a build by ID. Returns errs.ErrNotFound if the build is not found. Get(ctx context.Context, id string) (entity.Build, error) // Create creates a new build. The build must have a unique ID already assigned. diff --git a/submitqueue/extension/storage/mysql/BUILD.bazel b/submitqueue/extension/storage/mysql/BUILD.bazel index aac0822b..91859ebe 100644 --- a/submitqueue/extension/storage/mysql/BUILD.bazel +++ b/submitqueue/extension/storage/mysql/BUILD.bazel @@ -17,6 +17,7 @@ go_library( importpath = "github.com/uber/submitqueue/submitqueue/extension/storage/mysql", visibility = ["//visibility:public"], deps = [ + "//platform/errs:go_default_library", "//platform/metrics:go_default_library", "//submitqueue/entity:go_default_library", "//submitqueue/extension/storage:go_default_library", @@ -43,6 +44,7 @@ go_test( deps = [ "//platform/base/change:go_default_library", "//platform/base/mergestrategy:go_default_library", + "//platform/errs:go_default_library", "//submitqueue/entity:go_default_library", "//submitqueue/extension/storage:go_default_library", "@com_github_data_dog_go_sqlmock//:go_default_library", diff --git a/submitqueue/extension/storage/mysql/batch_dependent_store.go b/submitqueue/extension/storage/mysql/batch_dependent_store.go index 64de6431..840ebd95 100644 --- a/submitqueue/extension/storage/mysql/batch_dependent_store.go +++ b/submitqueue/extension/storage/mysql/batch_dependent_store.go @@ -24,6 +24,7 @@ import ( "github.com/go-sql-driver/mysql" "github.com/uber-go/tally" + "github.com/uber/submitqueue/platform/errs" "github.com/uber/submitqueue/platform/metrics" "github.com/uber/submitqueue/submitqueue/entity" "github.com/uber/submitqueue/submitqueue/extension/storage" @@ -39,7 +40,7 @@ func NewBatchDependentStore(db *sql.DB, scope tally.Scope) storage.BatchDependen return &batchDependentStore{db: db, scope: scope} } -// Get retrieves the batch dependent by batch ID. Returns ErrNotFound if the batch dependent is not found. +// Get retrieves the batch dependent by batch ID. Returns errs.ErrNotFound if the batch dependent is not found. func (s *batchDependentStore) Get(ctx context.Context, batchID string) (ret entity.BatchDependent, retErr error) { op := metrics.Begin(s.scope, "get") defer func() { op.Complete(retErr) }() @@ -53,7 +54,7 @@ func (s *batchDependentStore) Get(ctx context.Context, batchID string) (ret enti ).Scan(&bd.BatchID, &dependentsJSON, &bd.Version) if errors.Is(err, sql.ErrNoRows) { - return entity.BatchDependent{}, storage.WrapNotFound(err) + return entity.BatchDependent{}, fmt.Errorf("%w: %w", errs.ErrNotFound, err) } if err != nil { return entity.BatchDependent{}, fmt.Errorf("failed to get batch dependent entity batchID=%s from the database: %w", batchID, err) @@ -92,7 +93,7 @@ func (s *batchDependentStore) Create(ctx context.Context, batchDependent entity. } // UpdateDependents updates the dependents of a batch dependent and the version to newVersion -// if the current persisted version matches oldVersion. If versions do not match, returns ErrVersionMismatch. +// if the current persisted version matches oldVersion. If versions do not match, returns errs.ErrVersionMismatch. // Version arithmetic is owned by the caller; this is a pure conditional write. func (s *batchDependentStore) UpdateDependents(ctx context.Context, batchID string, oldVersion, newVersion int32, dependents []string) (retErr error) { op := metrics.Begin(s.scope, "update_dependents") @@ -125,7 +126,7 @@ func (s *batchDependentStore) UpdateDependents(ctx context.Context, batchID stri if rowsAffected != 1 { return fmt.Errorf( "version mismatch for batch dependent update: batchID=%q expected_version=%d: %w", - batchID, oldVersion, storage.ErrVersionMismatch, + batchID, oldVersion, errs.ErrVersionMismatch, ) } diff --git a/submitqueue/extension/storage/mysql/batch_dependent_store_test.go b/submitqueue/extension/storage/mysql/batch_dependent_store_test.go index 5988d72c..7d269bab 100644 --- a/submitqueue/extension/storage/mysql/batch_dependent_store_test.go +++ b/submitqueue/extension/storage/mysql/batch_dependent_store_test.go @@ -26,6 +26,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/uber/submitqueue/platform/errs" "github.com/uber/submitqueue/submitqueue/entity" "github.com/uber/submitqueue/submitqueue/extension/storage" ) @@ -79,7 +80,7 @@ func TestBatchDependentStore_Get(t *testing.T) { WillReturnError(sql.ErrNoRows) }, wantErr: true, - wantErrIs: storage.ErrNotFound, + wantErrIs: errs.ErrNotFound, }, { name: "query error", @@ -205,7 +206,7 @@ func TestBatchDependentStore_UpdateDependents(t *testing.T) { WillReturnResult(sqlmock.NewResult(0, 0)) }, wantErr: true, - wantErrIs: storage.ErrVersionMismatch, + wantErrIs: errs.ErrVersionMismatch, }, { name: "exec error", diff --git a/submitqueue/extension/storage/mysql/batch_store.go b/submitqueue/extension/storage/mysql/batch_store.go index 07020dac..dc99f7eb 100644 --- a/submitqueue/extension/storage/mysql/batch_store.go +++ b/submitqueue/extension/storage/mysql/batch_store.go @@ -25,6 +25,7 @@ import ( "github.com/go-sql-driver/mysql" "github.com/uber-go/tally" + "github.com/uber/submitqueue/platform/errs" "github.com/uber/submitqueue/platform/metrics" "github.com/uber/submitqueue/submitqueue/entity" "github.com/uber/submitqueue/submitqueue/extension/storage" @@ -40,7 +41,7 @@ func NewBatchStore(db *sql.DB, scope tally.Scope) storage.BatchStore { return &batchStore{db: db, scope: scope} } -// Get retrieves a batch by ID. Returns ErrNotFound if the batch is not found. +// Get retrieves a batch by ID. Returns errs.ErrNotFound if the batch is not found. func (s *batchStore) Get(ctx context.Context, id string) (ret entity.Batch, retErr error) { op := metrics.Begin(s.scope, "get") defer func() { op.Complete(retErr) }() @@ -55,7 +56,7 @@ func (s *batchStore) Get(ctx context.Context, id string) (ret entity.Batch, retE ).Scan(&batch.ID, &batch.Queue, &containsJSON, &dependenciesJSON, &batch.Score, &batch.State, &batch.Version) if errors.Is(err, sql.ErrNoRows) { - return entity.Batch{}, storage.WrapNotFound(err) + return entity.Batch{}, fmt.Errorf("%w: %w", errs.ErrNotFound, err) } if err != nil { return entity.Batch{}, fmt.Errorf("failed to get batch entity id=%s from the database: %w", id, err) @@ -103,7 +104,7 @@ func (s *batchStore) Create(ctx context.Context, batch entity.Batch) (retErr err } // UpdateState updates the state of a batch to newState and the version to newVersion -// if the current persisted version matches oldVersion. If versions do not match, returns ErrVersionMismatch. +// if the current persisted version matches oldVersion. If versions do not match, returns errs.ErrVersionMismatch. // Version arithmetic is owned by the caller; this is a pure conditional write. func (s *batchStore) UpdateState(ctx context.Context, id string, oldVersion, newVersion int32, newState entity.BatchState) (retErr error) { op := metrics.Begin(s.scope, "update_state") @@ -131,7 +132,7 @@ func (s *batchStore) UpdateState(ctx context.Context, id string, oldVersion, new if rowsAffected != 1 { return fmt.Errorf( "version mismatch for batch update: id=%q expected_version=%d newState=%v: %w", - id, oldVersion, newState, storage.ErrVersionMismatch, + id, oldVersion, newState, errs.ErrVersionMismatch, ) } @@ -139,7 +140,7 @@ func (s *batchStore) UpdateState(ctx context.Context, id string, oldVersion, new } // UpdateScoreAndState atomically updates the score and state of a batch and the version to newVersion -// if the current persisted version matches oldVersion. If versions do not match, returns ErrVersionMismatch. +// if the current persisted version matches oldVersion. If versions do not match, returns errs.ErrVersionMismatch. // Version arithmetic is owned by the caller; this is a pure conditional write. func (s *batchStore) UpdateScoreAndState(ctx context.Context, id string, oldVersion, newVersion int32, score float64, newState entity.BatchState) (retErr error) { op := metrics.Begin(s.scope, "update_score_and_state") @@ -167,7 +168,7 @@ func (s *batchStore) UpdateScoreAndState(ctx context.Context, id string, oldVers if rowsAffected != 1 { return fmt.Errorf( "version mismatch for batch update score and state: id=%q expected_version=%d score=%f newState=%v: %w", - id, oldVersion, score, newState, storage.ErrVersionMismatch, + id, oldVersion, score, newState, errs.ErrVersionMismatch, ) } diff --git a/submitqueue/extension/storage/mysql/batch_store_test.go b/submitqueue/extension/storage/mysql/batch_store_test.go index 1fca075a..5209a157 100644 --- a/submitqueue/extension/storage/mysql/batch_store_test.go +++ b/submitqueue/extension/storage/mysql/batch_store_test.go @@ -26,6 +26,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/uber/submitqueue/platform/errs" "github.com/uber/submitqueue/submitqueue/entity" "github.com/uber/submitqueue/submitqueue/extension/storage" ) @@ -85,7 +86,7 @@ func TestBatchStore_Get(t *testing.T) { WillReturnError(sql.ErrNoRows) }, wantErr: true, - wantErrIs: storage.ErrNotFound, + wantErrIs: errs.ErrNotFound, }, { name: "query error", @@ -227,7 +228,7 @@ func TestBatchStore_UpdateState(t *testing.T) { WillReturnResult(sqlmock.NewResult(0, 0)) }, wantErr: true, - wantErrIs: storage.ErrVersionMismatch, + wantErrIs: errs.ErrVersionMismatch, }, { name: "exec error", @@ -298,7 +299,7 @@ func TestBatchStore_UpdateScoreAndState(t *testing.T) { WillReturnResult(sqlmock.NewResult(0, 0)) }, wantErr: true, - wantErrIs: storage.ErrVersionMismatch, + wantErrIs: errs.ErrVersionMismatch, }, { name: "exec error", diff --git a/submitqueue/extension/storage/mysql/build_store.go b/submitqueue/extension/storage/mysql/build_store.go index 6d1b0184..fe00b7fd 100644 --- a/submitqueue/extension/storage/mysql/build_store.go +++ b/submitqueue/extension/storage/mysql/build_store.go @@ -23,6 +23,7 @@ import ( "github.com/go-sql-driver/mysql" "github.com/uber-go/tally" + "github.com/uber/submitqueue/platform/errs" "github.com/uber/submitqueue/platform/metrics" "github.com/uber/submitqueue/submitqueue/entity" "github.com/uber/submitqueue/submitqueue/extension/storage" @@ -38,7 +39,7 @@ func NewBuildStore(db *sql.DB, scope tally.Scope) storage.BuildStore { return &buildStore{db: db, scope: scope} } -// Get retrieves a build by ID. Returns ErrNotFound if the build is not found. +// Get retrieves a build by ID. Returns errs.ErrNotFound if the build is not found. func (s *buildStore) Get(ctx context.Context, id string) (ret entity.Build, retErr error) { op := metrics.Begin(s.scope, "get") defer func() { op.Complete(retErr) }() @@ -51,7 +52,7 @@ func (s *buildStore) Get(ctx context.Context, id string) (ret entity.Build, retE ).Scan(&build.ID, &build.BatchID, &build.Status) if errors.Is(err, sql.ErrNoRows) { - return entity.Build{}, storage.WrapNotFound(err) + return entity.Build{}, fmt.Errorf("%w: %w", errs.ErrNotFound, err) } if err != nil { return entity.Build{}, fmt.Errorf("failed to get build entity id=%s from the database: %w", id, err) @@ -80,7 +81,7 @@ func (s *buildStore) Create(ctx context.Context, build entity.Build) (retErr err return nil } -// UpdateStatus updates the status of a build. Returns ErrNotFound if the build is not found. +// UpdateStatus updates the status of a build. Returns errs.ErrNotFound if the build is not found. func (s *buildStore) UpdateStatus(ctx context.Context, id string, newStatus entity.BuildStatus) (retErr error) { op := metrics.Begin(s.scope, "update_status") defer func() { op.Complete(retErr) }() @@ -99,7 +100,7 @@ func (s *buildStore) UpdateStatus(ctx context.Context, id string, newStatus enti } if rowsAffected != 1 { - return storage.WrapNotFound(fmt.Errorf("build entity id=%s", id)) + return fmt.Errorf("build entity id=%s: %w", id, errs.ErrNotFound) } return nil diff --git a/submitqueue/extension/storage/mysql/build_store_test.go b/submitqueue/extension/storage/mysql/build_store_test.go index c89db986..a963bf33 100644 --- a/submitqueue/extension/storage/mysql/build_store_test.go +++ b/submitqueue/extension/storage/mysql/build_store_test.go @@ -25,6 +25,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/uber/submitqueue/platform/errs" "github.com/uber/submitqueue/submitqueue/entity" "github.com/uber/submitqueue/submitqueue/extension/storage" ) @@ -76,7 +77,7 @@ func TestBuildStore_Get(t *testing.T) { WillReturnError(sql.ErrNoRows) }, wantErr: true, - wantErrIs: storage.ErrNotFound, + wantErrIs: errs.ErrNotFound, }, { name: "query error", @@ -201,7 +202,7 @@ func TestBuildStore_UpdateStatus(t *testing.T) { WillReturnResult(sqlmock.NewResult(0, 0)) }, wantErr: true, - wantErrIs: storage.ErrNotFound, + wantErrIs: errs.ErrNotFound, }, { name: "exec error", diff --git a/submitqueue/extension/storage/mysql/request_log_store.go b/submitqueue/extension/storage/mysql/request_log_store.go index b19b2119..72ff25d9 100644 --- a/submitqueue/extension/storage/mysql/request_log_store.go +++ b/submitqueue/extension/storage/mysql/request_log_store.go @@ -23,6 +23,7 @@ import ( "github.com/uber-go/tally" + "github.com/uber/submitqueue/platform/errs" "github.com/uber/submitqueue/platform/metrics" "github.com/uber/submitqueue/submitqueue/entity" "github.com/uber/submitqueue/submitqueue/extension/storage" @@ -105,7 +106,7 @@ func (r *requestLogStore) List(ctx context.Context, requestID string) (ret []ent } if len(logs) == 0 { - return nil, fmt.Errorf("no request log records for request_id=%s: %w", requestID, storage.ErrNotFound) + return nil, fmt.Errorf("no request log records for request_id=%s: %w", requestID, errs.ErrNotFound) } return logs, nil diff --git a/submitqueue/extension/storage/mysql/request_log_store_test.go b/submitqueue/extension/storage/mysql/request_log_store_test.go index 577a9ab9..74f1b843 100644 --- a/submitqueue/extension/storage/mysql/request_log_store_test.go +++ b/submitqueue/extension/storage/mysql/request_log_store_test.go @@ -24,6 +24,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/uber/submitqueue/platform/errs" "github.com/uber/submitqueue/submitqueue/entity" "github.com/uber/submitqueue/submitqueue/extension/storage" ) @@ -131,7 +132,7 @@ func TestRequestLogStore_List(t *testing.T) { WillReturnRows(rows) }, wantErr: true, - wantErrIs: storage.ErrNotFound, + wantErrIs: errs.ErrNotFound, }, { name: "query error", diff --git a/submitqueue/extension/storage/mysql/request_queue_summary_store.go b/submitqueue/extension/storage/mysql/request_queue_summary_store.go index 8a292fe0..604a6984 100644 --- a/submitqueue/extension/storage/mysql/request_queue_summary_store.go +++ b/submitqueue/extension/storage/mysql/request_queue_summary_store.go @@ -24,6 +24,7 @@ import ( "github.com/go-sql-driver/mysql" "github.com/uber-go/tally" + "github.com/uber/submitqueue/platform/errs" "github.com/uber/submitqueue/platform/metrics" "github.com/uber/submitqueue/submitqueue/entity" "github.com/uber/submitqueue/submitqueue/extension/storage" @@ -78,7 +79,7 @@ func (s *requestQueueSummaryStore) Get(ctx context.Context, queue string, receiv WHERE queue = ? AND received_at_ms = ? AND request_id = ?`, queue, receivedAtMs, requestID, ).Scan(&ret.Queue, &ret.ReceivedAtMs, &ret.RequestID, &changeURIsJSON, &ret.Status, &ret.Version, &ret.LastError, &metadataJSON) if errors.Is(err, sql.ErrNoRows) { - return entity.RequestQueueSummary{}, storage.WrapNotFound(err) + return entity.RequestQueueSummary{}, fmt.Errorf("%w: %w", errs.ErrNotFound, err) } if err != nil { return entity.RequestQueueSummary{}, fmt.Errorf("failed to get queue summary queue=%s received_at_ms=%d request_id=%s: %w", queue, receivedAtMs, requestID, err) @@ -112,7 +113,7 @@ func (s *requestQueueSummaryStore) Update(ctx context.Context, summary entity.Re return fmt.Errorf("failed to get queue summary update rows request_id=%s: %w", summary.RequestID, err) } if rowsAffected != 1 { - return fmt.Errorf("queue summary request_id=%s expected_version=%d: %w", summary.RequestID, oldVersion, storage.ErrVersionMismatch) + return fmt.Errorf("queue summary request_id=%s expected_version=%d: %w", summary.RequestID, oldVersion, errs.ErrVersionMismatch) } return nil } diff --git a/submitqueue/extension/storage/mysql/request_queue_summary_store_test.go b/submitqueue/extension/storage/mysql/request_queue_summary_store_test.go index 1f04ba1e..c5a7bc36 100644 --- a/submitqueue/extension/storage/mysql/request_queue_summary_store_test.go +++ b/submitqueue/extension/storage/mysql/request_queue_summary_store_test.go @@ -25,6 +25,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/uber/submitqueue/platform/errs" "github.com/uber/submitqueue/submitqueue/entity" "github.com/uber/submitqueue/submitqueue/extension/storage" ) @@ -150,7 +151,7 @@ func TestRequestQueueSummaryStore_Get(t *testing.T) { WillReturnError(sql.ErrNoRows) }, wantErr: true, - wantErrIs: storage.ErrNotFound, + wantErrIs: errs.ErrNotFound, }, } @@ -215,7 +216,7 @@ func TestRequestQueueSummaryStore_Update(t *testing.T) { WillReturnResult(sqlmock.NewResult(0, 0)) }, wantErr: true, - wantErrIs: storage.ErrVersionMismatch, + wantErrIs: errs.ErrVersionMismatch, }, { name: "exec error", diff --git a/submitqueue/extension/storage/mysql/request_store.go b/submitqueue/extension/storage/mysql/request_store.go index fd542c9d..be4adf24 100644 --- a/submitqueue/extension/storage/mysql/request_store.go +++ b/submitqueue/extension/storage/mysql/request_store.go @@ -24,6 +24,7 @@ import ( "github.com/go-sql-driver/mysql" "github.com/uber-go/tally" + "github.com/uber/submitqueue/platform/errs" "github.com/uber/submitqueue/platform/metrics" "github.com/uber/submitqueue/submitqueue/entity" "github.com/uber/submitqueue/submitqueue/extension/storage" @@ -39,7 +40,7 @@ func NewRequestStore(db *sql.DB, scope tally.Scope) storage.RequestStore { return &requestStore{db: db, scope: scope} } -// Get retrieves a land request by ID. Returns ErrNotFound if the request is not found. +// Get retrieves a land request by ID. Returns errs.ErrNotFound if the request is not found. func (r *requestStore) Get(ctx context.Context, id string) (ret entity.Request, retErr error) { op := metrics.Begin(r.scope, "get") defer func() { op.Complete(retErr) }() @@ -53,7 +54,7 @@ func (r *requestStore) Get(ctx context.Context, id string) (ret entity.Request, ).Scan(&req.ID, &req.Queue, &changeURIsJSON, &req.LandStrategy, &req.State, &req.Version) if errors.Is(err, sql.ErrNoRows) { - return entity.Request{}, storage.WrapNotFound(err) + return entity.Request{}, fmt.Errorf("%w: %w", errs.ErrNotFound, err) } if err != nil { return entity.Request{}, fmt.Errorf("failed to get request entity id=%s from the database: %w", id, err) @@ -94,7 +95,7 @@ func (r *requestStore) Create(ctx context.Context, request entity.Request) (retE } // UpdateState updates the state of a land request to newState and the version to newVersion -// if the current persisted version matches oldVersion. If versions do not match, returns ErrVersionMismatch. +// if the current persisted version matches oldVersion. If versions do not match, returns errs.ErrVersionMismatch. // Version arithmetic is owned by the caller; this is a pure conditional write. func (r *requestStore) UpdateState(ctx context.Context, id string, oldVersion, newVersion int32, newState entity.RequestState) (retErr error) { op := metrics.Begin(r.scope, "update_state") @@ -122,7 +123,7 @@ func (r *requestStore) UpdateState(ctx context.Context, id string, oldVersion, n if rowsAffected != 1 { return fmt.Errorf( "version mismatch for request update: id=%q expected_version=%d newState=%v: %w", - id, oldVersion, newState, storage.ErrVersionMismatch, + id, oldVersion, newState, errs.ErrVersionMismatch, ) } diff --git a/submitqueue/extension/storage/mysql/request_store_test.go b/submitqueue/extension/storage/mysql/request_store_test.go index c13668b8..5fcb42da 100644 --- a/submitqueue/extension/storage/mysql/request_store_test.go +++ b/submitqueue/extension/storage/mysql/request_store_test.go @@ -28,6 +28,7 @@ import ( "github.com/uber/submitqueue/platform/base/change" "github.com/uber/submitqueue/platform/base/mergestrategy" + "github.com/uber/submitqueue/platform/errs" "github.com/uber/submitqueue/submitqueue/entity" "github.com/uber/submitqueue/submitqueue/extension/storage" ) @@ -84,7 +85,7 @@ func TestRequestStore_Get(t *testing.T) { WillReturnError(sql.ErrNoRows) }, wantErr: true, - wantErrIs: storage.ErrNotFound, + wantErrIs: errs.ErrNotFound, }, { name: "query error", @@ -225,7 +226,7 @@ func TestRequestStore_UpdateState(t *testing.T) { WillReturnResult(sqlmock.NewResult(0, 0)) }, wantErr: true, - wantErrIs: storage.ErrVersionMismatch, + wantErrIs: errs.ErrVersionMismatch, }, { name: "exec error", diff --git a/submitqueue/extension/storage/mysql/request_summary_store.go b/submitqueue/extension/storage/mysql/request_summary_store.go index 36849dea..fc9c25a5 100644 --- a/submitqueue/extension/storage/mysql/request_summary_store.go +++ b/submitqueue/extension/storage/mysql/request_summary_store.go @@ -24,6 +24,7 @@ import ( "github.com/go-sql-driver/mysql" "github.com/uber-go/tally" + "github.com/uber/submitqueue/platform/errs" "github.com/uber/submitqueue/platform/metrics" "github.com/uber/submitqueue/submitqueue/entity" "github.com/uber/submitqueue/submitqueue/extension/storage" @@ -87,7 +88,7 @@ func (s *requestSummaryStore) Get(ctx context.Context, requestID string) (ret en &ret.LastError, &metadataJSON, ) if errors.Is(err, sql.ErrNoRows) { - return entity.RequestSummary{}, storage.WrapNotFound(err) + return entity.RequestSummary{}, fmt.Errorf("%w: %w", errs.ErrNotFound, err) } if err != nil { return entity.RequestSummary{}, fmt.Errorf("failed to get request summary request_id=%s: %w", requestID, err) @@ -126,7 +127,7 @@ func (s *requestSummaryStore) Update(ctx context.Context, summary entity.Request return fmt.Errorf("failed to get request summary update rows request_id=%s: %w", summary.RequestID, err) } if rowsAffected != 1 { - return fmt.Errorf("request summary request_id=%s expected_version=%d: %w", summary.RequestID, oldVersion, storage.ErrVersionMismatch) + return fmt.Errorf("request summary request_id=%s expected_version=%d: %w", summary.RequestID, oldVersion, errs.ErrVersionMismatch) } return nil diff --git a/submitqueue/extension/storage/mysql/request_summary_store_test.go b/submitqueue/extension/storage/mysql/request_summary_store_test.go index 9399794b..6661db89 100644 --- a/submitqueue/extension/storage/mysql/request_summary_store_test.go +++ b/submitqueue/extension/storage/mysql/request_summary_store_test.go @@ -25,6 +25,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/uber/submitqueue/platform/errs" "github.com/uber/submitqueue/submitqueue/entity" "github.com/uber/submitqueue/submitqueue/extension/storage" ) @@ -160,7 +161,7 @@ func TestRequestSummaryStore_Get(t *testing.T) { WillReturnError(sql.ErrNoRows) }, wantErr: true, - wantErrIs: storage.ErrNotFound, + wantErrIs: errs.ErrNotFound, }, { name: "query error", @@ -232,7 +233,7 @@ func TestRequestSummaryStore_Update(t *testing.T) { WillReturnResult(sqlmock.NewResult(0, 0)) }, wantErr: true, - wantErrIs: storage.ErrVersionMismatch, + wantErrIs: errs.ErrVersionMismatch, }, { name: "exec error", diff --git a/submitqueue/extension/storage/request_log_store.go b/submitqueue/extension/storage/request_log_store.go index 3ab2c5cc..434ea5e6 100644 --- a/submitqueue/extension/storage/request_log_store.go +++ b/submitqueue/extension/storage/request_log_store.go @@ -29,6 +29,6 @@ type RequestLogStore interface { Insert(ctx context.Context, log entity.RequestLog) error // List retrieves all request log records for a given request ID, ordered by timestamp ascending. - // Returns ErrNotFound if no records exist for the given request ID. + // Returns errs.ErrNotFound if no records exist for the given request ID. List(ctx context.Context, requestID string) ([]entity.RequestLog, error) } diff --git a/submitqueue/extension/storage/request_queue_summary_store.go b/submitqueue/extension/storage/request_queue_summary_store.go index 68acf211..4301b09f 100644 --- a/submitqueue/extension/storage/request_queue_summary_store.go +++ b/submitqueue/extension/storage/request_queue_summary_store.go @@ -51,11 +51,11 @@ type RequestQueueSummaryStore interface { // Create inserts summary and returns ErrAlreadyExists when its full primary key already exists. Create(ctx context.Context, summary entity.RequestQueueSummary) error - // Get returns the row identified by its full primary key, or ErrNotFound when absent. + // Get returns the row identified by its full primary key, or errs.ErrNotFound when absent. Get(ctx context.Context, queue string, receivedAtMs int64, requestID string) (entity.RequestQueueSummary, error) // Update conditionally replaces mutable fields when the persisted projection version equals oldVersion. - // The store writes newVersion exactly as supplied and returns ErrVersionMismatch when the guard does not match. + // The store writes newVersion exactly as supplied and returns errs.ErrVersionMismatch when the guard does not match. Update(ctx context.Context, summary entity.RequestQueueSummary, oldVersion, newVersion int32) error // List returns at most query.Limit rows ordered by received_at_ms descending, then request_id descending. diff --git a/submitqueue/extension/storage/request_store.go b/submitqueue/extension/storage/request_store.go index fa3e7b27..ba41219c 100644 --- a/submitqueue/extension/storage/request_store.go +++ b/submitqueue/extension/storage/request_store.go @@ -24,7 +24,7 @@ import ( // RequestStore is an interface that defines methods for managing land requests in the database. type RequestStore interface { - // Get retrieves a land request by ID. Returns ErrNotFound if the request is not found. + // Get retrieves a land request by ID. Returns errs.ErrNotFound if the request is not found. Get(ctx context.Context, id string) (entity.Request, error) // Create creates a new land request. The request must have a unique ID already assigned. @@ -32,7 +32,7 @@ type RequestStore interface { Create(ctx context.Context, request entity.Request) error // UpdateState updates the state of a land request to newState and the version to newVersion - // if the current persisted version matches oldVersion. If versions do not match, returns ErrVersionMismatch. + // if the current persisted version matches oldVersion. If versions do not match, returns errs.ErrVersionMismatch. // Version arithmetic is owned by the caller; the store performs a pure conditional write. UpdateState(ctx context.Context, id string, oldVersion, newVersion int32, newState entity.RequestState) error } diff --git a/submitqueue/extension/storage/request_summary_store.go b/submitqueue/extension/storage/request_summary_store.go index 5e2a657a..88570464 100644 --- a/submitqueue/extension/storage/request_summary_store.go +++ b/submitqueue/extension/storage/request_summary_store.go @@ -28,10 +28,10 @@ type RequestSummaryStore interface { // The caller owns retry identity and decides whether an existing row is an identical retry or a conflict. Create(ctx context.Context, summary entity.RequestSummary) error - // Get returns the summary for requestID, or ErrNotFound when absent. + // Get returns the summary for requestID, or errs.ErrNotFound when absent. Get(ctx context.Context, requestID string) (entity.RequestSummary, error) // Update conditionally replaces the mutable status fields when the persisted projection version equals oldVersion. - // The store writes newVersion exactly as supplied and returns ErrVersionMismatch when the guard does not match. + // The store writes newVersion exactly as supplied and returns errs.ErrVersionMismatch when the guard does not match. Update(ctx context.Context, summary entity.RequestSummary, oldVersion, newVersion int32) error } diff --git a/submitqueue/extension/storage/storage.go b/submitqueue/extension/storage/storage.go index c9506e5a..4e0644d1 100644 --- a/submitqueue/extension/storage/storage.go +++ b/submitqueue/extension/storage/storage.go @@ -18,30 +18,11 @@ package storage import ( "errors" - "fmt" ) -// ErrNotFound is returned by storage implementations when the requested record is not found in the database. -var ErrNotFound = errors.New("record not found") - -// IsNotFound returns true if any error in the error chain is a ErrNotFound. -func IsNotFound(err error) bool { - return errors.Is(err, ErrNotFound) -} - -// WrapNotFound wraps ErrNotFound with the original error from the storage implementation. -func WrapNotFound(err error) error { - return fmt.Errorf("%w: %w", ErrNotFound, err) -} - // ErrAlreadyExists is returned by storage implementations when attempting to create a record with an ID that already exists. var ErrAlreadyExists = errors.New("record already exists") -// ErrVersionMismatch is returned by storage implementations when the expected entity version does not match the current version of the object. -// This is used to implement an optimistic locking mechanism, allowing multiple clients to update the same entity concurrently -// and either retry or implement idempotent operations. -var ErrVersionMismatch = errors.New("version mismatch") - // Storage is a factory interface that aggregates all entity stores into a single injectable dependency. type Storage interface { // GetRequestStore returns the RequestStore instance. diff --git a/submitqueue/gateway/controller/BUILD.bazel b/submitqueue/gateway/controller/BUILD.bazel index 282d8950..73ead367 100644 --- a/submitqueue/gateway/controller/BUILD.bazel +++ b/submitqueue/gateway/controller/BUILD.bazel @@ -53,7 +53,6 @@ go_test( "//platform/extension/messagequeue/mock:go_default_library", "//submitqueue/core/topickey:go_default_library", "//submitqueue/entity:go_default_library", - "//submitqueue/extension/queueconfig:go_default_library", "//submitqueue/extension/queueconfig/mock:go_default_library", "//submitqueue/extension/storage:go_default_library", "//submitqueue/extension/storage/mock:go_default_library", diff --git a/submitqueue/gateway/controller/cancel.go b/submitqueue/gateway/controller/cancel.go index 298345b1..b1f93dc8 100644 --- a/submitqueue/gateway/controller/cancel.go +++ b/submitqueue/gateway/controller/cancel.go @@ -16,6 +16,7 @@ package controller import ( "context" + "errors" "fmt" "time" @@ -83,7 +84,7 @@ func (c *CancelController) Cancel(ctx context.Context, req entity.CancelRequest) // Verify the sqid exists before recording intent or publishing. if _, err := c.requestSummaryStore.Get(ctx, req.ID); err != nil { - if storage.IsNotFound(err) { + if errors.Is(err, errs.ErrNotFound) { c.metricsScope.Counter("cancel_request_not_found").Inc(1) return errs.NewUserError(&RequestNotFoundError{Sqid: req.ID}) } diff --git a/submitqueue/gateway/controller/land.go b/submitqueue/gateway/controller/land.go index 6175fcc0..fc8cff3b 100644 --- a/submitqueue/gateway/controller/land.go +++ b/submitqueue/gateway/controller/land.go @@ -104,7 +104,7 @@ func (c *LandController) Land(ctx context.Context, req entity.LandRequest) (resu queue := req.Queue if _, err := c.queueConfigs.Get(ctx, queue); err != nil { - if errors.Is(err, queueconfig.ErrNotFound) { + if errors.Is(err, errs.ErrNotFound) { return entity.LandResult{}, errs.NewUserError(&UnrecognizedQueueError{Queue: queue}) } return entity.LandResult{}, fmt.Errorf("LandController failed to look up queue %q: %w", queue, err) diff --git a/submitqueue/gateway/controller/land_test.go b/submitqueue/gateway/controller/land_test.go index 80fcc081..d97bf81b 100644 --- a/submitqueue/gateway/controller/land_test.go +++ b/submitqueue/gateway/controller/land_test.go @@ -32,7 +32,6 @@ import ( queuemock "github.com/uber/submitqueue/platform/extension/messagequeue/mock" "github.com/uber/submitqueue/submitqueue/core/topickey" "github.com/uber/submitqueue/submitqueue/entity" - "github.com/uber/submitqueue/submitqueue/extension/queueconfig" qcmock "github.com/uber/submitqueue/submitqueue/extension/queueconfig/mock" "github.com/uber/submitqueue/submitqueue/extension/storage" storagemock "github.com/uber/submitqueue/submitqueue/extension/storage/mock" @@ -272,7 +271,7 @@ func TestLand_ReturnsUnrecognizedQueueWhenStoreReportsNotFound(t *testing.T) { cnt := countermock.NewMockCounter(ctrl) qcs := qcmock.NewMockStore(ctrl) - qcs.EXPECT().Get(gomock.Any(), "missing-queue").Return(entity.QueueConfig{}, queueconfig.ErrNotFound) + qcs.EXPECT().Get(gomock.Any(), "missing-queue").Return(entity.QueueConfig{}, errs.ErrNotFound) controller := NewLandController(zap.NewNop().Sugar(), tally.NoopScope, cnt, noopStorage(ctrl), qcs, newTestRegistryWithNoopPublisher(t, ctrl)) ctx := context.Background() @@ -365,7 +364,7 @@ func TestLand_PublishesToQueue(t *testing.T) { ), queueStore.EXPECT().Get(gomock.Any(), "test-queue", gomock.Any(), "test-queue/123").DoAndReturn( func(context.Context, string, int64, string) (entity.RequestQueueSummary, error) { - return entity.RequestQueueSummary{}, storage.ErrNotFound + return entity.RequestQueueSummary{}, errs.ErrNotFound }, ), uriStore.EXPECT().Create(gomock.Any(), gomock.Any()).DoAndReturn( diff --git a/submitqueue/gateway/controller/list.go b/submitqueue/gateway/controller/list.go index c629777e..36735902 100644 --- a/submitqueue/gateway/controller/list.go +++ b/submitqueue/gateway/controller/list.go @@ -71,7 +71,7 @@ func (c *ListController) List(ctx context.Context, req entity.ListRequest) (resu return entity.ListResult{}, fmt.Errorf("ListController invalid queue: %w", err) } if _, err := c.queueConfigs.Get(ctx, req.Queue); err != nil { - if errors.Is(err, queueconfig.ErrNotFound) { + if errors.Is(err, errs.ErrNotFound) { return entity.ListResult{}, errs.NewUserError(&UnrecognizedQueueError{Queue: req.Queue}) } return entity.ListResult{}, fmt.Errorf("ListController failed to look up queue %q: %w", req.Queue, err) diff --git a/submitqueue/gateway/controller/list_test.go b/submitqueue/gateway/controller/list_test.go index 3ec52e52..76fb1a16 100644 --- a/submitqueue/gateway/controller/list_test.go +++ b/submitqueue/gateway/controller/list_test.go @@ -23,8 +23,8 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "github.com/uber-go/tally" + "github.com/uber/submitqueue/platform/errs" "github.com/uber/submitqueue/submitqueue/entity" - "github.com/uber/submitqueue/submitqueue/extension/queueconfig" qcmock "github.com/uber/submitqueue/submitqueue/extension/queueconfig/mock" "github.com/uber/submitqueue/submitqueue/extension/storage" storagemock "github.com/uber/submitqueue/submitqueue/extension/storage/mock" @@ -111,7 +111,7 @@ func TestList_Errors(t *testing.T) { queueConfigs := qcmock.NewMockStore(ctrl) if tt.request.Queue != "" { if tt.wantUnknown { - queueConfigs.EXPECT().Get(gomock.Any(), tt.request.Queue).Return(entity.QueueConfig{}, queueconfig.ErrNotFound) + queueConfigs.EXPECT().Get(gomock.Any(), tt.request.Queue).Return(entity.QueueConfig{}, errs.ErrNotFound) } else { queueConfigs.EXPECT().Get(gomock.Any(), tt.request.Queue).Return(entity.QueueConfig{}, nil) } diff --git a/submitqueue/gateway/controller/request_history.go b/submitqueue/gateway/controller/request_history.go index d306d4a5..4d2c95a6 100644 --- a/submitqueue/gateway/controller/request_history.go +++ b/submitqueue/gateway/controller/request_history.go @@ -16,6 +16,7 @@ package controller import ( "context" + "errors" "fmt" "sort" "strconv" @@ -58,7 +59,7 @@ func (c *RequestHistoryController) GetRequestHistoryByID(ctx context.Context, re logs, err := c.requestLogStore.List(ctx, req.ID) if err != nil { - if storage.IsNotFound(err) { + if errors.Is(err, errs.ErrNotFound) { return nil, errs.NewUserError(&RequestNotFoundError{Sqid: req.ID}) } return nil, fmt.Errorf("GetRequestHistoryByID failed to list request logs sqid=%s: %w", req.ID, err) @@ -95,7 +96,7 @@ func (c *RequestHistoryController) GetRequestHistoryByChangeURI(ctx context.Cont for _, mapping := range mappings { logs, err := c.requestLogStore.List(ctx, mapping.RequestID) if err != nil { - if storage.IsNotFound(err) { + if errors.Is(err, errs.ErrNotFound) { continue } return nil, fmt.Errorf("GetRequestHistoryByChangeURI failed to list request logs change_uri=%s sqid=%s: %w", req.ChangeURI, mapping.RequestID, err) diff --git a/submitqueue/gateway/controller/request_history_test.go b/submitqueue/gateway/controller/request_history_test.go index 2fd814ae..5ad25510 100644 --- a/submitqueue/gateway/controller/request_history_test.go +++ b/submitqueue/gateway/controller/request_history_test.go @@ -24,7 +24,6 @@ import ( "github.com/uber-go/tally" "github.com/uber/submitqueue/platform/errs" "github.com/uber/submitqueue/submitqueue/entity" - "github.com/uber/submitqueue/submitqueue/extension/storage" storagemock "github.com/uber/submitqueue/submitqueue/extension/storage/mock" "go.uber.org/mock/gomock" "go.uber.org/zap" @@ -64,7 +63,7 @@ func TestGetRequestHistoryByChangeURI(t *testing.T) { }, nil) logStore.EXPECT().List(gomock.Any(), "queue/10").Return([]entity.RequestLog{{RequestID: "queue/10", TimestampMs: 10, Status: entity.RequestStatusLanded}}, nil) logStore.EXPECT().List(gomock.Any(), "b/2").Return([]entity.RequestLog{{RequestID: "b/2", TimestampMs: 2, Status: entity.RequestStatusStarted}}, nil) - logStore.EXPECT().List(gomock.Any(), "missing/3").Return(nil, storage.ErrNotFound) + logStore.EXPECT().List(gomock.Any(), "missing/3").Return(nil, errs.ErrNotFound) logStore.EXPECT().List(gomock.Any(), "queue/1").Return([]entity.RequestLog{{RequestID: "queue/1", TimestampMs: 1, Status: entity.RequestStatusAccepted}}, nil) logStore.EXPECT().List(gomock.Any(), "a/2").Return([]entity.RequestLog{{RequestID: "a/2", TimestampMs: 2, Status: entity.RequestStatusError}}, nil) @@ -114,7 +113,7 @@ func TestHistoryErrors(t *testing.T) { { name: "sqid not found", setup: func(logStore *storagemock.MockRequestLogStore, _ *storagemock.MockRequestURIStore) { - logStore.EXPECT().List(gomock.Any(), "missing/1").Return(nil, storage.ErrNotFound) + logStore.EXPECT().List(gomock.Any(), "missing/1").Return(nil, errs.ErrNotFound) }, call: func(c *RequestHistoryController) error { _, err := c.GetRequestHistoryByID(context.Background(), entity.GetRequestHistoryByIDRequest{ID: "missing/1"}) @@ -161,7 +160,7 @@ func TestHistoryErrors(t *testing.T) { name: "all mapped logs absent", setup: func(logStore *storagemock.MockRequestLogStore, uriStore *storagemock.MockRequestURIStore) { uriStore.EXPECT().ListByURI(gomock.Any(), "uri", 101).Return([]entity.RequestURI{{RequestID: "queue/1"}}, nil) - logStore.EXPECT().List(gomock.Any(), "queue/1").Return(nil, storage.ErrNotFound) + logStore.EXPECT().List(gomock.Any(), "queue/1").Return(nil, errs.ErrNotFound) }, call: func(c *RequestHistoryController) error { _, err := c.GetRequestHistoryByChangeURI(context.Background(), entity.GetRequestHistoryByChangeURIRequest{ChangeURI: "uri"}) diff --git a/submitqueue/gateway/controller/request_summary.go b/submitqueue/gateway/controller/request_summary.go index ff604b19..5de41872 100644 --- a/submitqueue/gateway/controller/request_summary.go +++ b/submitqueue/gateway/controller/request_summary.go @@ -16,6 +16,7 @@ package controller import ( "context" + "errors" "fmt" "github.com/uber-go/tally" @@ -55,7 +56,7 @@ func (c *RequestSummaryController) GetRequestSummaryByID(ctx context.Context, re summary, err := c.requestSummaryStore.Get(ctx, req.ID) if err != nil { - if storage.IsNotFound(err) { + if errors.Is(err, errs.ErrNotFound) { return entity.RequestSummary{}, errs.NewUserError(&RequestNotFoundError{Sqid: req.ID}) } return entity.RequestSummary{}, fmt.Errorf("GetRequestSummaryByID failed to get request summary sqid=%s: %w", req.ID, err) @@ -95,7 +96,7 @@ func (c *RequestSummaryController) GetRequestSummaryByChangeURI(ctx context.Cont for _, mapping := range mappings { summary, err := c.requestSummaryStore.Get(ctx, mapping.RequestID) if err != nil { - if storage.IsNotFound(err) { + if errors.Is(err, errs.ErrNotFound) { return nil, &InternalConsistencyError{Message: fmt.Sprintf("request summary missing for mapped change URI %q and sqid %q", req.ChangeURI, mapping.RequestID)} } return nil, fmt.Errorf("GetRequestSummaryByChangeURI failed to get request summary change_uri=%s sqid=%s: %w", req.ChangeURI, mapping.RequestID, err) diff --git a/submitqueue/gateway/controller/request_summary_test.go b/submitqueue/gateway/controller/request_summary_test.go index f54d78a8..8d9780c7 100644 --- a/submitqueue/gateway/controller/request_summary_test.go +++ b/submitqueue/gateway/controller/request_summary_test.go @@ -24,7 +24,6 @@ import ( "github.com/uber-go/tally" "github.com/uber/submitqueue/platform/errs" "github.com/uber/submitqueue/submitqueue/entity" - "github.com/uber/submitqueue/submitqueue/extension/storage" storagemock "github.com/uber/submitqueue/submitqueue/extension/storage/mock" "go.uber.org/mock/gomock" "go.uber.org/zap" @@ -109,7 +108,7 @@ func TestStatusErrors(t *testing.T) { { name: "sqid not found", setup: func(summaryStore *storagemock.MockRequestSummaryStore, _ *storagemock.MockRequestURIStore) { - summaryStore.EXPECT().Get(gomock.Any(), "missing/1").Return(entity.RequestSummary{}, storage.ErrNotFound) + summaryStore.EXPECT().Get(gomock.Any(), "missing/1").Return(entity.RequestSummary{}, errs.ErrNotFound) }, call: func(c *RequestSummaryController) error { _, err := c.GetRequestSummaryByID(context.Background(), entity.GetRequestSummaryByIDRequest{ID: "missing/1"}) @@ -171,7 +170,7 @@ func TestStatusErrors(t *testing.T) { name: "mapped summary missing", setup: func(summaryStore *storagemock.MockRequestSummaryStore, uriStore *storagemock.MockRequestURIStore) { uriStore.EXPECT().ListByURI(gomock.Any(), "uri", 101).Return([]entity.RequestURI{{RequestID: "missing/1"}}, nil) - summaryStore.EXPECT().Get(gomock.Any(), "missing/1").Return(entity.RequestSummary{}, storage.ErrNotFound) + summaryStore.EXPECT().Get(gomock.Any(), "missing/1").Return(entity.RequestSummary{}, errs.ErrNotFound) }, call: func(c *RequestSummaryController) error { _, err := c.GetRequestSummaryByChangeURI(context.Background(), entity.GetRequestSummaryByChangeURIRequest{ChangeURI: "uri"}) diff --git a/submitqueue/gateway/controller/storage_fixture_test.go b/submitqueue/gateway/controller/storage_fixture_test.go index 832aecdc..2f2cf1a5 100644 --- a/submitqueue/gateway/controller/storage_fixture_test.go +++ b/submitqueue/gateway/controller/storage_fixture_test.go @@ -19,6 +19,7 @@ import ( "fmt" "sync" + "github.com/uber/submitqueue/platform/errs" "github.com/uber/submitqueue/submitqueue/entity" "github.com/uber/submitqueue/submitqueue/extension/storage" storagemock "github.com/uber/submitqueue/submitqueue/extension/storage/mock" @@ -68,7 +69,7 @@ func newControllerStorageFixture(ctrl *gomock.Controller) *controllerStorageFixt defer fixture.mu.Unlock() summary, ok := fixture.summaries[requestID] if !ok { - return entity.RequestSummary{}, storage.ErrNotFound + return entity.RequestSummary{}, errs.ErrNotFound } return summary, nil }).AnyTimes() @@ -77,10 +78,10 @@ func newControllerStorageFixture(ctrl *gomock.Controller) *controllerStorageFixt defer fixture.mu.Unlock() current, ok := fixture.summaries[summary.RequestID] if !ok { - return storage.ErrNotFound + return errs.ErrNotFound } if current.Version != oldVersion { - return storage.ErrVersionMismatch + return errs.ErrVersionMismatch } summary.Version = newVersion fixture.summaries[summary.RequestID] = summary @@ -102,7 +103,7 @@ func newControllerStorageFixture(ctrl *gomock.Controller) *controllerStorageFixt defer fixture.mu.Unlock() summary, ok := fixture.queueSummaries[queueSummaryTestKey(queue, receivedAtMs, requestID)] if !ok { - return entity.RequestQueueSummary{}, storage.ErrNotFound + return entity.RequestQueueSummary{}, errs.ErrNotFound } return summary, nil }).AnyTimes() @@ -112,10 +113,10 @@ func newControllerStorageFixture(ctrl *gomock.Controller) *controllerStorageFixt key := queueSummaryTestKey(summary.Queue, summary.ReceivedAtMs, summary.RequestID) current, ok := fixture.queueSummaries[key] if !ok { - return storage.ErrNotFound + return errs.ErrNotFound } if current.Version != oldVersion { - return storage.ErrVersionMismatch + return errs.ErrVersionMismatch } summary.Version = newVersion fixture.queueSummaries[key] = summary diff --git a/submitqueue/orchestrator/controller/batch/BUILD.bazel b/submitqueue/orchestrator/controller/batch/BUILD.bazel index 8ded51d9..78e53966 100644 --- a/submitqueue/orchestrator/controller/batch/BUILD.bazel +++ b/submitqueue/orchestrator/controller/batch/BUILD.bazel @@ -8,6 +8,7 @@ go_library( deps = [ "//platform/base/messagequeue:go_default_library", "//platform/consumer:go_default_library", + "//platform/errs:go_default_library", "//platform/extension/counter:go_default_library", "//platform/metrics:go_default_library", "//submitqueue/core/request:go_default_library", @@ -29,6 +30,7 @@ go_test( "//platform/base/mergestrategy:go_default_library", "//platform/base/messagequeue:go_default_library", "//platform/consumer:go_default_library", + "//platform/errs:go_default_library", "//platform/extension/counter/mock:go_default_library", "//platform/extension/messagequeue/mock:go_default_library", "//submitqueue/core/topickey:go_default_library", @@ -36,7 +38,6 @@ go_test( "//submitqueue/extension/conflict:go_default_library", "//submitqueue/extension/conflict/all:go_default_library", "//submitqueue/extension/conflict/mock:go_default_library", - "//submitqueue/extension/storage:go_default_library", "//submitqueue/extension/storage/mock:go_default_library", "@com_github_stretchr_testify//assert:go_default_library", "@com_github_stretchr_testify//require:go_default_library", diff --git a/submitqueue/orchestrator/controller/batch/batch.go b/submitqueue/orchestrator/controller/batch/batch.go index 9bb13e85..50e3c0cd 100644 --- a/submitqueue/orchestrator/controller/batch/batch.go +++ b/submitqueue/orchestrator/controller/batch/batch.go @@ -22,6 +22,7 @@ import ( "github.com/uber-go/tally" entityqueue "github.com/uber/submitqueue/platform/base/messagequeue" "github.com/uber/submitqueue/platform/consumer" + "github.com/uber/submitqueue/platform/errs" "github.com/uber/submitqueue/platform/extension/counter" "github.com/uber/submitqueue/platform/metrics" corerequest "github.com/uber/submitqueue/submitqueue/core/request" @@ -224,7 +225,7 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) (r // // The CAS below collapses that window. Whichever of batch.UpdateState(..., // RequestStateBatched) and cancel.markCancelling(... RequestStateCancelling) - // reaches storage first wins; the loser sees storage.ErrVersionMismatch: + // reaches storage first wins; the loser sees errs.ErrVersionMismatch: // - If cancel won: this CAS fails. We ack the message (cancel will drive R // to its terminal state on its own; no batch is needed). The reverse-index // entry above becomes a dangling BatchDependent — tolerated per the @@ -258,7 +259,7 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) (r // ErrVersionMismatch == cancel (or another writer) advanced R first. Ack // the message: there is nothing for us to do, and retrying would not help // since the new state of R is now visible to the cancel pipeline. - if errors.Is(err, storage.ErrVersionMismatch) { + if errors.Is(err, errs.ErrVersionMismatch) { c.metricsScope.Counter("request_claim_lost_race").Inc(1) c.logger.Infow("abandoning batch creation; request advanced concurrently (likely cancel)", "request_id", request.ID, diff --git a/submitqueue/orchestrator/controller/batch/batch_test.go b/submitqueue/orchestrator/controller/batch/batch_test.go index fe02f62f..9f02cb81 100644 --- a/submitqueue/orchestrator/controller/batch/batch_test.go +++ b/submitqueue/orchestrator/controller/batch/batch_test.go @@ -28,6 +28,7 @@ import ( "github.com/uber/submitqueue/platform/base/mergestrategy" entityqueue "github.com/uber/submitqueue/platform/base/messagequeue" "github.com/uber/submitqueue/platform/consumer" + "github.com/uber/submitqueue/platform/errs" countermock "github.com/uber/submitqueue/platform/extension/counter/mock" queuemock "github.com/uber/submitqueue/platform/extension/messagequeue/mock" "github.com/uber/submitqueue/submitqueue/core/topickey" @@ -35,7 +36,6 @@ import ( "github.com/uber/submitqueue/submitqueue/extension/conflict" "github.com/uber/submitqueue/submitqueue/extension/conflict/all" conflictmock "github.com/uber/submitqueue/submitqueue/extension/conflict/mock" - "github.com/uber/submitqueue/submitqueue/extension/storage" storagemock "github.com/uber/submitqueue/submitqueue/extension/storage/mock" "go.uber.org/mock/gomock" "go.uber.org/zap/zaptest" @@ -477,7 +477,7 @@ func TestController_Process_HaltedShortCircuit(t *testing.T) { // Race-lost path: the cancel controller's markCancelling CAS landed first, // so the batch controller's request-claim CAS (Validated → Batched) fails -// with storage.ErrVersionMismatch. The controller must ack the message (the +// with errs.ErrVersionMismatch. The controller must ack the message (the // cancel pipeline now owns the request) and must NOT call BatchStore.Create // or publish to the score topic. // @@ -503,7 +503,7 @@ func TestController_Process_CASLostToCancel(t *testing.T) { mockReqStore.EXPECT().Get(gomock.Any(), request.ID).Return(request, nil) mockReqStore.EXPECT().UpdateState( gomock.Any(), request.ID, request.Version, request.Version+1, entity.RequestStateBatched, - ).Return(fmt.Errorf("cas: %w", storage.ErrVersionMismatch)) + ).Return(fmt.Errorf("cas: %w", errs.ErrVersionMismatch)) mockStorage := storagemock.NewMockStorage(ctrl) mockStorage.EXPECT().GetBatchStore().Return(mockBatchStore).AnyTimes() diff --git a/submitqueue/orchestrator/controller/cancel/BUILD.bazel b/submitqueue/orchestrator/controller/cancel/BUILD.bazel index 770a2989..0974189f 100644 --- a/submitqueue/orchestrator/controller/cancel/BUILD.bazel +++ b/submitqueue/orchestrator/controller/cancel/BUILD.bazel @@ -24,6 +24,7 @@ go_test( deps = [ "//platform/base/messagequeue:go_default_library", "//platform/consumer:go_default_library", + "//platform/errs:go_default_library", "//platform/extension/messagequeue/mock:go_default_library", "//submitqueue/core/topickey:go_default_library", "//submitqueue/entity:go_default_library", diff --git a/submitqueue/orchestrator/controller/cancel/cancel.go b/submitqueue/orchestrator/controller/cancel/cancel.go index 1faaa499..0a2d99d8 100644 --- a/submitqueue/orchestrator/controller/cancel/cancel.go +++ b/submitqueue/orchestrator/controller/cancel/cancel.go @@ -46,10 +46,10 @@ // Cancelling batch re-publishes to TopicKeySpeculate (a cheap no-op nudge // the speculate controller absorbs). // -// Concurrent producers surface as storage.ErrVersionMismatch; the controller +// Concurrent producers surface as errs.ErrVersionMismatch; the controller // returns the wrapped error as-is and relies on the base controller layer to // classify it as retryable so the next attempt sees the new state and takes -// the other branch. storage.ErrNotFound on the initial Get (the start +// the other branch. errs.ErrNotFound on the initial Get (the start // controller has not yet persisted the request) is returned as-is for the // same reason. package cancel @@ -160,7 +160,7 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) er // version and state) on success, or the original request on the idempotent path // where the prior delivery already wrote Cancelling. // -// storage.ErrVersionMismatch (a concurrent writer — most likely conclude +// errs.ErrVersionMismatch (a concurrent writer, most likely conclude // observing a batch transition) is returned as-is for the base controller to // classify and retry; the next attempt re-fetches and re-evaluates (it may now // be terminal, in which case the top-level terminal-check acks). @@ -210,7 +210,7 @@ func (c *Controller) findActiveBatch(ctx context.Context, request entity.Request // cancelRequest performs the terminal CAS (Cancelling → Cancelled) for a request // that is not part of any active batch, and emits the RequestStatusCancelled log -// entry. storage.ErrVersionMismatch here means a concurrent writer (typically +// entry. errs.ErrVersionMismatch here means a concurrent writer (typically // conclude after a racing batch terminal transition) advanced the request between // our mark-cancelling CAS and this terminal CAS — returned as-is for the base // controller to classify and retry; the next pass will observe the new state @@ -267,7 +267,7 @@ func (c *Controller) cancelBatch(ctx context.Context, batch entity.Batch) error newVersion := batch.Version + 1 if err := c.store.GetBatchStore().UpdateState(ctx, batch.ID, batch.Version, newVersion, entity.BatchStateCancelling); err != nil { c.metricsScope.Counter("batch_update_errors").Inc(1) - // storage.ErrVersionMismatch here means the batch advanced concurrently + // errs.ErrVersionMismatch here means the batch advanced concurrently // (e.g. speculate / merge progressed). Returned as-is for the base // controller to classify and retry; the re-fetch will see the new state // and either short-circuit (already terminal) or attempt the transition diff --git a/submitqueue/orchestrator/controller/cancel/cancel_test.go b/submitqueue/orchestrator/controller/cancel/cancel_test.go index b625c949..0c8394f0 100644 --- a/submitqueue/orchestrator/controller/cancel/cancel_test.go +++ b/submitqueue/orchestrator/controller/cancel/cancel_test.go @@ -24,6 +24,7 @@ import ( "github.com/uber-go/tally" entityqueue "github.com/uber/submitqueue/platform/base/messagequeue" "github.com/uber/submitqueue/platform/consumer" + "github.com/uber/submitqueue/platform/errs" queuemock "github.com/uber/submitqueue/platform/extension/messagequeue/mock" "github.com/uber/submitqueue/submitqueue/core/topickey" "github.com/uber/submitqueue/submitqueue/entity" @@ -111,7 +112,7 @@ func TestProcess_RequestNotFound_Retryable(t *testing.T) { registry, _ := newRegistry(t, ctrl) reqStore := storagemock.NewMockRequestStore(ctrl) - reqStore.EXPECT().Get(gomock.Any(), "q/1").Return(entity.Request{}, storage.ErrNotFound) + reqStore.EXPECT().Get(gomock.Any(), "q/1").Return(entity.Request{}, errs.ErrNotFound) store := storagemock.NewMockStorage(ctrl) store.EXPECT().GetRequestStore().Return(reqStore).AnyTimes() @@ -119,7 +120,7 @@ func TestProcess_RequestNotFound_Retryable(t *testing.T) { controller := newController(t, store, registry) err := controller.Process(context.Background(), newDelivery(t, ctrl, cancelPayload(t, "q/1", ""), "q/1")) require.Error(t, err) - assert.ErrorIs(t, err, storage.ErrNotFound) + assert.ErrorIs(t, err, errs.ErrNotFound) } func TestProcess_CancelsUnbatchedRequest(t *testing.T) { @@ -188,7 +189,7 @@ func TestProcess_AlreadyCancelling_SkipsMarkCancelling(t *testing.T) { // TestProcess_MarkCancellingVersionMismatch_Retryable covers the case where the // first CAS (mark-cancelling) loses to a concurrent writer. The underlying -// storage.ErrVersionMismatch must be preserved in the error chain so the base +// errs.ErrVersionMismatch must be preserved in the error chain so the base // controller can classify it as retryable; the next pass re-fetches and // re-evaluates (possibly observing a terminal state and acking). func TestProcess_MarkCancellingVersionMismatch_Retryable(t *testing.T) { @@ -201,7 +202,7 @@ func TestProcess_MarkCancellingVersionMismatch_Retryable(t *testing.T) { ID: "q/1", Queue: "q", State: entity.RequestStateStarted, Version: 2, }, nil) reqStore.EXPECT().UpdateState(gomock.Any(), "q/1", int32(2), int32(3), entity.RequestStateCancelling). - Return(storage.ErrVersionMismatch) + Return(errs.ErrVersionMismatch) store := storagemock.NewMockStorage(ctrl) store.EXPECT().GetRequestStore().Return(reqStore).AnyTimes() @@ -209,7 +210,7 @@ func TestProcess_MarkCancellingVersionMismatch_Retryable(t *testing.T) { controller := newController(t, store, registry) err := controller.Process(context.Background(), newDelivery(t, ctrl, cancelPayload(t, "q/1", ""), "q/1")) require.Error(t, err) - assert.ErrorIs(t, err, storage.ErrVersionMismatch) + assert.ErrorIs(t, err, errs.ErrVersionMismatch) } func TestProcess_UnbatchedVersionMismatch_Retryable(t *testing.T) { @@ -224,7 +225,7 @@ func TestProcess_UnbatchedVersionMismatch_Retryable(t *testing.T) { gomock.InOrder( reqStore.EXPECT().UpdateState(gomock.Any(), "q/1", int32(2), int32(3), entity.RequestStateCancelling).Return(nil), reqStore.EXPECT().UpdateState(gomock.Any(), "q/1", int32(3), int32(4), entity.RequestStateCancelled). - Return(storage.ErrVersionMismatch), + Return(errs.ErrVersionMismatch), ) batchStore := storagemock.NewMockBatchStore(ctrl) @@ -237,7 +238,7 @@ func TestProcess_UnbatchedVersionMismatch_Retryable(t *testing.T) { controller := newController(t, store, registry) err := controller.Process(context.Background(), newDelivery(t, ctrl, cancelPayload(t, "q/1", ""), "q/1")) require.Error(t, err) - assert.ErrorIs(t, err, storage.ErrVersionMismatch) + assert.ErrorIs(t, err, errs.ErrVersionMismatch) } // TestProcess_BatchPath_HandsOffToSpeculate asserts the entire batch path: @@ -341,7 +342,7 @@ func TestProcess_BatchAlreadyCancelling_RepublishesToSpeculate(t *testing.T) { // TestProcess_BatchIntentVersionMismatch_Retryable covers the case where the // intent CAS (mark batch Cancelling) loses to a concurrent batch state -// transition (e.g. speculate just advanced it). storage.ErrVersionMismatch +// transition (e.g. speculate just advanced it). errs.ErrVersionMismatch // must be preserved so the base controller can classify the failure as // retryable. func TestProcess_BatchIntentVersionMismatch_Retryable(t *testing.T) { @@ -358,7 +359,7 @@ func TestProcess_BatchIntentVersionMismatch_Retryable(t *testing.T) { batchStore := storagemock.NewMockBatchStore(ctrl) batchStore.EXPECT().GetByQueueAndStates(gomock.Any(), "q", gomock.Any()).Return([]entity.Batch{batch}, nil) batchStore.EXPECT().UpdateState(gomock.Any(), batch.ID, int32(1), int32(2), entity.BatchStateCancelling). - Return(storage.ErrVersionMismatch) + Return(errs.ErrVersionMismatch) store := storagemock.NewMockStorage(ctrl) store.EXPECT().GetRequestStore().Return(reqStore).AnyTimes() @@ -367,7 +368,7 @@ func TestProcess_BatchIntentVersionMismatch_Retryable(t *testing.T) { controller := newController(t, store, registry) err := controller.Process(context.Background(), newDelivery(t, ctrl, cancelPayload(t, "q/1", ""), "q/1")) require.Error(t, err) - assert.ErrorIs(t, err, storage.ErrVersionMismatch) + assert.ErrorIs(t, err, errs.ErrVersionMismatch) } func TestProcess_DeserializeError(t *testing.T) { diff --git a/submitqueue/orchestrator/controller/conclude/BUILD.bazel b/submitqueue/orchestrator/controller/conclude/BUILD.bazel index 13fa7175..e3a26f5f 100644 --- a/submitqueue/orchestrator/controller/conclude/BUILD.bazel +++ b/submitqueue/orchestrator/controller/conclude/BUILD.bazel @@ -27,7 +27,6 @@ go_test( "//platform/extension/messagequeue/mock:go_default_library", "//submitqueue/core/topickey:go_default_library", "//submitqueue/entity:go_default_library", - "//submitqueue/extension/storage:go_default_library", "//submitqueue/extension/storage/mock:go_default_library", "@com_github_stretchr_testify//assert:go_default_library", "@com_github_stretchr_testify//require:go_default_library", diff --git a/submitqueue/orchestrator/controller/conclude/conclude_test.go b/submitqueue/orchestrator/controller/conclude/conclude_test.go index 461f8b5d..16496863 100644 --- a/submitqueue/orchestrator/controller/conclude/conclude_test.go +++ b/submitqueue/orchestrator/controller/conclude/conclude_test.go @@ -28,7 +28,6 @@ import ( queuemock "github.com/uber/submitqueue/platform/extension/messagequeue/mock" "github.com/uber/submitqueue/submitqueue/core/topickey" "github.com/uber/submitqueue/submitqueue/entity" - "github.com/uber/submitqueue/submitqueue/extension/storage" storagemock "github.com/uber/submitqueue/submitqueue/extension/storage/mock" "go.uber.org/mock/gomock" "go.uber.org/zap/zaptest" @@ -339,7 +338,7 @@ func TestController_Process(t *testing.T) { mockRequestStore.EXPECT().Get(gomock.Any(), "test-queue/1").Return(entity.Request{ ID: "test-queue/1", Version: 2, State: entity.RequestStateProcessing, }, nil) - mockRequestStore.EXPECT().UpdateState(gomock.Any(), "test-queue/1", int32(2), int32(3), entity.RequestStateLanded).Return(storage.ErrVersionMismatch) + mockRequestStore.EXPECT().UpdateState(gomock.Any(), "test-queue/1", int32(2), int32(3), entity.RequestStateLanded).Return(errs.ErrVersionMismatch) mockStorage := storagemock.NewMockStorage(ctrl) mockStorage.EXPECT().GetBatchStore().Return(mockBatchStore).AnyTimes() diff --git a/submitqueue/orchestrator/controller/dlq/BUILD.bazel b/submitqueue/orchestrator/controller/dlq/BUILD.bazel index 08a52c22..752d964f 100644 --- a/submitqueue/orchestrator/controller/dlq/BUILD.bazel +++ b/submitqueue/orchestrator/controller/dlq/BUILD.bazel @@ -16,6 +16,7 @@ go_library( deps = [ "//api/runway/messagequeue:go_default_library", "//platform/consumer:go_default_library", + "//platform/errs:go_default_library", "//platform/metrics:go_default_library", "//submitqueue/core/request:go_default_library", "//submitqueue/entity:go_default_library", @@ -47,7 +48,6 @@ go_test( "//platform/extension/messagequeue/mock:go_default_library", "//submitqueue/core/topickey:go_default_library", "//submitqueue/entity:go_default_library", - "//submitqueue/extension/storage:go_default_library", "//submitqueue/extension/storage/mock:go_default_library", "@com_github_stretchr_testify//assert:go_default_library", "@com_github_stretchr_testify//require:go_default_library", diff --git a/submitqueue/orchestrator/controller/dlq/README.md b/submitqueue/orchestrator/controller/dlq/README.md index a0a028f3..67e38a0b 100644 --- a/submitqueue/orchestrator/controller/dlq/README.md +++ b/submitqueue/orchestrator/controller/dlq/README.md @@ -16,9 +16,9 @@ DLQ consumers are wired with `errs.AlwaysRetryableProcessor` and a very high `Re The recognised error condition is handled explicitly in `dlq.go`: -- `storage.ErrNotFound` → logged at warn and treated as success. The request or batch never persisted; there is nothing to reconcile. +- `errs.ErrNotFound` -> logged at warn and treated as success. The request or batch never persisted; there is nothing to reconcile. -Everything else — including `storage.ErrVersionMismatch` on the CAS — is returned plain and, after the always-retryable processor wrap, redelivered until it either succeeds or hits the attempt cap. There is no point in pre-classifying retryability at this layer when the processor forces every non-nil error retryable anyway. +Everything else, including `errs.ErrVersionMismatch` on the CAS, is returned plain and, after the always-retryable processor wrap, redelivered until it either succeeds or hits the attempt cap. There is no point in pre-classifying retryability at this layer when the processor forces every non-nil error retryable anyway. ## Request log entries are published to Gateway diff --git a/submitqueue/orchestrator/controller/dlq/buildsignal.go b/submitqueue/orchestrator/controller/dlq/buildsignal.go index 58a6c5f2..473fa1ea 100644 --- a/submitqueue/orchestrator/controller/dlq/buildsignal.go +++ b/submitqueue/orchestrator/controller/dlq/buildsignal.go @@ -21,6 +21,7 @@ import ( "github.com/uber-go/tally" "github.com/uber/submitqueue/platform/consumer" + "github.com/uber/submitqueue/platform/errs" "github.com/uber/submitqueue/platform/metrics" "github.com/uber/submitqueue/submitqueue/entity" "github.com/uber/submitqueue/submitqueue/extension/storage" @@ -97,7 +98,7 @@ func (c *buildSignalController) Process(ctx context.Context, delivery consumer.D build, err := c.store.GetBuildStore().Get(ctx, bid.ID) if err != nil { - if errors.Is(err, storage.ErrNotFound) { + if errors.Is(err, errs.ErrNotFound) { // The build was never persisted (e.g. the build controller crashed // before Create). There is no batch to reconcile from this signal — // any associated batch should be reconciled from its own DLQ. diff --git a/submitqueue/orchestrator/controller/dlq/buildsignal_test.go b/submitqueue/orchestrator/controller/dlq/buildsignal_test.go index 41da60a2..7ee93bda 100644 --- a/submitqueue/orchestrator/controller/dlq/buildsignal_test.go +++ b/submitqueue/orchestrator/controller/dlq/buildsignal_test.go @@ -21,9 +21,9 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "github.com/uber/submitqueue/platform/consumer" + "github.com/uber/submitqueue/platform/errs" "github.com/uber/submitqueue/submitqueue/core/topickey" "github.com/uber/submitqueue/submitqueue/entity" - "github.com/uber/submitqueue/submitqueue/extension/storage" storagemock "github.com/uber/submitqueue/submitqueue/extension/storage/mock" "go.uber.org/mock/gomock" "go.uber.org/zap/zaptest" @@ -83,7 +83,7 @@ func TestDLQBuildSignalController_Process_BuildNotFoundIsNoOp(t *testing.T) { ctrl := gomock.NewController(t) buildStore := storagemock.NewMockBuildStore(ctrl) - buildStore.EXPECT().Get(gomock.Any(), "build-1").Return(entity.Build{}, storage.ErrNotFound) + buildStore.EXPECT().Get(gomock.Any(), "build-1").Return(entity.Build{}, errs.ErrNotFound) store := storagemock.NewMockStorage(ctrl) store.EXPECT().GetBuildStore().Return(buildStore).AnyTimes() diff --git a/submitqueue/orchestrator/controller/dlq/dlq.go b/submitqueue/orchestrator/controller/dlq/dlq.go index 5de49810..d09cfee8 100644 --- a/submitqueue/orchestrator/controller/dlq/dlq.go +++ b/submitqueue/orchestrator/controller/dlq/dlq.go @@ -39,6 +39,7 @@ import ( "fmt" "github.com/uber/submitqueue/platform/consumer" + "github.com/uber/submitqueue/platform/errs" requestcore "github.com/uber/submitqueue/submitqueue/core/request" "github.com/uber/submitqueue/submitqueue/entity" "github.com/uber/submitqueue/submitqueue/extension/storage" @@ -75,7 +76,7 @@ func TopicKey(main consumer.TopicKey) consumer.TopicKey { func failRequest(ctx context.Context, store storage.Storage, registry consumer.TopicRegistry, logger *zap.SugaredLogger, requestID, lastError string) error { request, err := store.GetRequestStore().Get(ctx, requestID) if err != nil { - if errors.Is(err, storage.ErrNotFound) { + if errors.Is(err, errs.ErrNotFound) { logger.Warnw("dlq reconcile: request not found, skipping", "request_id", requestID, ) @@ -138,7 +139,7 @@ func failRequest(ctx context.Context, store storage.Storage, registry consumer.T func failBatch(ctx context.Context, store storage.Storage, registry consumer.TopicRegistry, logger *zap.SugaredLogger, batchID, lastError string) error { batch, err := store.GetBatchStore().Get(ctx, batchID) if err != nil { - if errors.Is(err, storage.ErrNotFound) { + if errors.Is(err, errs.ErrNotFound) { logger.Warnw("dlq reconcile: batch not found, skipping", "batch_id", batchID, ) diff --git a/submitqueue/orchestrator/controller/dlq/dlq_test.go b/submitqueue/orchestrator/controller/dlq/dlq_test.go index ad8f6ff3..422a8258 100644 --- a/submitqueue/orchestrator/controller/dlq/dlq_test.go +++ b/submitqueue/orchestrator/controller/dlq/dlq_test.go @@ -25,7 +25,6 @@ import ( "github.com/uber/submitqueue/platform/consumer" "github.com/uber/submitqueue/platform/errs" "github.com/uber/submitqueue/submitqueue/entity" - "github.com/uber/submitqueue/submitqueue/extension/storage" storagemock "github.com/uber/submitqueue/submitqueue/extension/storage/mock" "go.uber.org/mock/gomock" "go.uber.org/zap/zaptest" @@ -148,7 +147,7 @@ func TestFailRequest_NotFoundIsNoOp(t *testing.T) { ctrl := gomock.NewController(t) requestStore := storagemock.NewMockRequestStore(ctrl) - requestStore.EXPECT().Get(gomock.Any(), "q/1").Return(entity.Request{}, storage.ErrNotFound) + requestStore.EXPECT().Get(gomock.Any(), "q/1").Return(entity.Request{}, errs.ErrNotFound) store := storagemock.NewMockStorage(ctrl) store.EXPECT().GetRequestStore().Return(requestStore).AnyTimes() @@ -289,7 +288,7 @@ func TestFailBatch_NotFoundIsNoOp(t *testing.T) { ctrl := gomock.NewController(t) batchStore := storagemock.NewMockBatchStore(ctrl) - batchStore.EXPECT().Get(gomock.Any(), "q/batch/1").Return(entity.Batch{}, storage.ErrNotFound) + batchStore.EXPECT().Get(gomock.Any(), "q/batch/1").Return(entity.Batch{}, errs.ErrNotFound) store := storagemock.NewMockStorage(ctrl) store.EXPECT().GetBatchStore().Return(batchStore).AnyTimes() diff --git a/submitqueue/orchestrator/controller/speculate/BUILD.bazel b/submitqueue/orchestrator/controller/speculate/BUILD.bazel index 39e36977..20f2c3e1 100644 --- a/submitqueue/orchestrator/controller/speculate/BUILD.bazel +++ b/submitqueue/orchestrator/controller/speculate/BUILD.bazel @@ -8,6 +8,7 @@ go_library( deps = [ "//platform/base/messagequeue:go_default_library", "//platform/consumer:go_default_library", + "//platform/errs:go_default_library", "//platform/metrics:go_default_library", "//submitqueue/core/topickey:go_default_library", "//submitqueue/entity:go_default_library", @@ -28,7 +29,6 @@ go_test( "//platform/extension/messagequeue/mock:go_default_library", "//submitqueue/core/topickey:go_default_library", "//submitqueue/entity:go_default_library", - "//submitqueue/extension/storage:go_default_library", "//submitqueue/extension/storage/mock:go_default_library", "@com_github_stretchr_testify//assert:go_default_library", "@com_github_stretchr_testify//require:go_default_library", diff --git a/submitqueue/orchestrator/controller/speculate/speculate.go b/submitqueue/orchestrator/controller/speculate/speculate.go index f2d2b21b..9eae99bd 100644 --- a/submitqueue/orchestrator/controller/speculate/speculate.go +++ b/submitqueue/orchestrator/controller/speculate/speculate.go @@ -22,6 +22,7 @@ import ( "github.com/uber-go/tally" entityqueue "github.com/uber/submitqueue/platform/base/messagequeue" "github.com/uber/submitqueue/platform/consumer" + "github.com/uber/submitqueue/platform/errs" "github.com/uber/submitqueue/platform/metrics" "github.com/uber/submitqueue/submitqueue/core/topickey" "github.com/uber/submitqueue/submitqueue/entity" @@ -287,7 +288,7 @@ func (c *Controller) failOnDependency(ctx context.Context, batch entity.Batch, d // terminal self-heal branch, which re-runs the dependent fan-out and the // conclude publish for already-Cancelled batches. // -// storage.ErrVersionMismatch on the terminal CAS is returned as-is for the +// errs.ErrVersionMismatch on the terminal CAS is returned as-is for the // base controller to classify as retryable; the redelivery will land in the // self-heal branch and complete the fan-out. func (c *Controller) cancelBatch(ctx context.Context, batch entity.Batch) error { @@ -339,7 +340,7 @@ func (c *Controller) cancelBatch(ctx context.Context, batch entity.Batch) error func (c *Controller) cancelBuild(ctx context.Context, batch entity.Batch) error { build, err := c.store.GetBuildStore().Get(ctx, batch.ID) if err != nil { - if errors.Is(err, storage.ErrNotFound) { + if errors.Is(err, errs.ErrNotFound) { metrics.NamedCounter(c.metricsScope, opName, "cancel_build_not_found", 1) return nil } diff --git a/submitqueue/orchestrator/controller/speculate/speculate_test.go b/submitqueue/orchestrator/controller/speculate/speculate_test.go index 3cdbd7ce..edb43460 100644 --- a/submitqueue/orchestrator/controller/speculate/speculate_test.go +++ b/submitqueue/orchestrator/controller/speculate/speculate_test.go @@ -28,7 +28,6 @@ import ( queuemock "github.com/uber/submitqueue/platform/extension/messagequeue/mock" "github.com/uber/submitqueue/submitqueue/core/topickey" "github.com/uber/submitqueue/submitqueue/entity" - "github.com/uber/submitqueue/submitqueue/extension/storage" storagemock "github.com/uber/submitqueue/submitqueue/extension/storage/mock" "go.uber.org/mock/gomock" "go.uber.org/zap/zaptest" @@ -455,7 +454,7 @@ func TestController_Process_CancellingNoBuildYet(t *testing.T) { batchStore.EXPECT().UpdateState(gomock.Any(), batch.ID, int32(1), int32(2), entity.BatchStateCancelled).Return(nil) buildStore := storagemock.NewMockBuildStore(ctrl) - buildStore.EXPECT().Get(gomock.Any(), batch.ID).Return(entity.Build{}, storage.ErrNotFound) + buildStore.EXPECT().Get(gomock.Any(), batch.ID).Return(entity.Build{}, errs.ErrNotFound) // No UpdateStatus expected. depStore := storagemock.NewMockBatchDependentStore(ctrl) @@ -485,7 +484,7 @@ func TestController_Process_CancellingNoDependents(t *testing.T) { batchStore.EXPECT().UpdateState(gomock.Any(), batch.ID, int32(1), int32(2), entity.BatchStateCancelled).Return(nil) buildStore := storagemock.NewMockBuildStore(ctrl) - buildStore.EXPECT().Get(gomock.Any(), batch.ID).Return(entity.Build{}, storage.ErrNotFound) + buildStore.EXPECT().Get(gomock.Any(), batch.ID).Return(entity.Build{}, errs.ErrNotFound) depStore := storagemock.NewMockBatchDependentStore(ctrl) depStore.EXPECT().Get(gomock.Any(), batch.ID).Return(entity.BatchDependent{BatchID: batch.ID, Dependents: []string{}, Version: 1}, nil) @@ -514,7 +513,7 @@ func TestController_Process_CancellingNoDependents(t *testing.T) { require.NoError(t, runProcess(t, ctrl, controller, batch.ID)) } -// storage.ErrVersionMismatch on the terminal CAS must surface as an error +// errs.ErrVersionMismatch on the terminal CAS must surface as an error // with the underlying sentinel in the chain so the base controller can // classify it as retryable. The dependent fan-out and conclude publish must // NOT run if the terminal CAS failed — on redelivery the self-heal branch @@ -526,10 +525,10 @@ func TestController_Process_CancellingTerminalCASVersionMismatch(t *testing.T) { batchStore := storagemock.NewMockBatchStore(ctrl) batchStore.EXPECT().Get(gomock.Any(), batch.ID).Return(batch, nil) batchStore.EXPECT().UpdateState(gomock.Any(), batch.ID, int32(1), int32(2), entity.BatchStateCancelled). - Return(storage.ErrVersionMismatch) + Return(errs.ErrVersionMismatch) buildStore := storagemock.NewMockBuildStore(ctrl) - buildStore.EXPECT().Get(gomock.Any(), batch.ID).Return(entity.Build{}, storage.ErrNotFound) + buildStore.EXPECT().Get(gomock.Any(), batch.ID).Return(entity.Build{}, errs.ErrNotFound) store := storagemock.NewMockStorage(ctrl) store.EXPECT().GetBatchStore().Return(batchStore).AnyTimes() @@ -554,7 +553,7 @@ func TestController_Process_CancellingTerminalCASVersionMismatch(t *testing.T) { err = runProcess(t, ctrl, controller, batch.ID) require.Error(t, err) - assert.ErrorIs(t, err, storage.ErrVersionMismatch) + assert.ErrorIs(t, err, errs.ErrVersionMismatch) } // An unrecognized state must surface as an error so the message is nacked diff --git a/submitqueue/orchestrator/controller/validate/BUILD.bazel b/submitqueue/orchestrator/controller/validate/BUILD.bazel index 0b0beff7..b1ae2a6e 100644 --- a/submitqueue/orchestrator/controller/validate/BUILD.bazel +++ b/submitqueue/orchestrator/controller/validate/BUILD.bazel @@ -39,7 +39,6 @@ go_test( "//submitqueue/core/topickey:go_default_library", "//submitqueue/entity:go_default_library", "//submitqueue/extension/changeprovider/mock:go_default_library", - "//submitqueue/extension/storage:go_default_library", "//submitqueue/extension/storage/mock:go_default_library", "//submitqueue/extension/validator:go_default_library", "//submitqueue/extension/validator/mock:go_default_library", diff --git a/submitqueue/orchestrator/controller/validate/validate.go b/submitqueue/orchestrator/controller/validate/validate.go index 390969d2..c10b774f 100644 --- a/submitqueue/orchestrator/controller/validate/validate.go +++ b/submitqueue/orchestrator/controller/validate/validate.go @@ -249,7 +249,7 @@ func (c *Controller) checkDuplicate(ctx context.Context, request entity.Request) seenOwners[rec.RequestID] = struct{}{} owner, err := c.store.GetRequestStore().Get(ctx, rec.RequestID) - if errors.Is(err, storage.ErrNotFound) { + if errors.Is(err, errs.ErrNotFound) { continue } if err != nil { diff --git a/submitqueue/orchestrator/controller/validate/validate_test.go b/submitqueue/orchestrator/controller/validate/validate_test.go index c4526c5d..6973f141 100644 --- a/submitqueue/orchestrator/controller/validate/validate_test.go +++ b/submitqueue/orchestrator/controller/validate/validate_test.go @@ -33,7 +33,6 @@ import ( "github.com/uber/submitqueue/submitqueue/core/topickey" "github.com/uber/submitqueue/submitqueue/entity" changeprovidermock "github.com/uber/submitqueue/submitqueue/extension/changeprovider/mock" - "github.com/uber/submitqueue/submitqueue/extension/storage" storagemock "github.com/uber/submitqueue/submitqueue/extension/storage/mock" "github.com/uber/submitqueue/submitqueue/extension/validator" validatormock "github.com/uber/submitqueue/submitqueue/extension/validator/mock" @@ -458,7 +457,7 @@ func TestController_Process_DuplicateDetection(t *testing.T) { mockReqStore.EXPECT().Get(gomock.Any(), id).Return(req, nil) } for id := range tt.ownerNotFound { - mockReqStore.EXPECT().Get(gomock.Any(), id).Return(entity.Request{}, storage.WrapNotFound(fmt.Errorf("missing"))) + mockReqStore.EXPECT().Get(gomock.Any(), id).Return(entity.Request{}, fmt.Errorf("%w: missing", errs.ErrNotFound)) } for id, e := range tt.ownerErr { mockReqStore.EXPECT().Get(gomock.Any(), id).Return(entity.Request{}, e) diff --git a/test/integration/stovepipe/extension/storage/BUILD.bazel b/test/integration/stovepipe/extension/storage/BUILD.bazel index b1f4654b..11cc043b 100644 --- a/test/integration/stovepipe/extension/storage/BUILD.bazel +++ b/test/integration/stovepipe/extension/storage/BUILD.bazel @@ -6,6 +6,7 @@ go_library( importpath = "github.com/uber/submitqueue/test/integration/stovepipe/extension/storage", visibility = ["//visibility:public"], deps = [ + "//platform/errs:go_default_library", "//stovepipe/entity:go_default_library", "//stovepipe/extension/storage:go_default_library", "//test/testutil:go_default_library", diff --git a/test/integration/stovepipe/extension/storage/mysql/BUILD.bazel b/test/integration/stovepipe/extension/storage/mysql/BUILD.bazel index 7125598c..eda58233 100644 --- a/test/integration/stovepipe/extension/storage/mysql/BUILD.bazel +++ b/test/integration/stovepipe/extension/storage/mysql/BUILD.bazel @@ -12,6 +12,7 @@ go_test( "integration", ], deps = [ + "//platform/errs:go_default_library", "//stovepipe/entity:go_default_library", "//stovepipe/extension/storage:go_default_library", "//stovepipe/extension/storage/mysql:go_default_library", diff --git a/test/integration/stovepipe/extension/storage/mysql/storage_test.go b/test/integration/stovepipe/extension/storage/mysql/storage_test.go index 8ab93de1..e24aa9fc 100644 --- a/test/integration/stovepipe/extension/storage/mysql/storage_test.go +++ b/test/integration/stovepipe/extension/storage/mysql/storage_test.go @@ -23,6 +23,7 @@ import ( "github.com/stretchr/testify/require" "github.com/stretchr/testify/suite" "github.com/uber-go/tally" + "github.com/uber/submitqueue/platform/errs" "github.com/uber/submitqueue/stovepipe/entity" "github.com/uber/submitqueue/stovepipe/extension/storage" mysqlstorage "github.com/uber/submitqueue/stovepipe/extension/storage/mysql" @@ -114,7 +115,7 @@ func (s *MySQLRequestStoreSuite) TestCreateAndGetWithProcessFields() { func (s *MySQLRequestStoreSuite) TestGetNotFound() { _, err := s.store.Get(s.ctx, "request/monorepo/main/does-not-exist") - require.True(s.T(), storage.IsNotFound(err)) + require.ErrorIs(s.T(), err, errs.ErrNotFound) } func (s *MySQLRequestStoreSuite) TestUpdateCAS() { @@ -142,13 +143,13 @@ func (s *MySQLRequestStoreSuite) TestUpdateCAS() { // Stale CAS: oldVersion 1 no longer matches the stored version (2). err = s.store.Update(s.ctx, updated, 1, 2) - require.ErrorIs(s.T(), err, storage.ErrVersionMismatch) + require.ErrorIs(s.T(), err, errs.ErrVersionMismatch) } func (s *MySQLRequestStoreSuite) TestUpdateNotFoundIsVersionMismatch() { missing := entity.Request{ID: "request/monorepo/main/missing", State: entity.RequestStateAccepted} err := s.store.Update(s.ctx, missing, 1, 2) - require.ErrorIs(s.T(), err, storage.ErrVersionMismatch) + require.ErrorIs(s.T(), err, errs.ErrVersionMismatch) } func (s *MySQLRequestStoreSuite) TestCreateDuplicateID() { @@ -179,7 +180,7 @@ func (s *MySQLRequestStoreSuite) TestURIMappingCreateAndGet() { func (s *MySQLRequestStoreSuite) TestGetIDByURINotFound() { _, err := s.uriStore.GetIDByURI(s.ctx, "monorepo/main", "git://remote/monorepo/main/unmapped") - require.True(s.T(), storage.IsNotFound(err)) + require.ErrorIs(s.T(), err, errs.ErrNotFound) } func (s *MySQLRequestStoreSuite) TestURIMappingDuplicate() { diff --git a/test/integration/stovepipe/extension/storage/suite.go b/test/integration/stovepipe/extension/storage/suite.go index 42965ee9..bb6206a7 100644 --- a/test/integration/stovepipe/extension/storage/suite.go +++ b/test/integration/stovepipe/extension/storage/suite.go @@ -20,6 +20,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "github.com/stretchr/testify/suite" + "github.com/uber/submitqueue/platform/errs" "github.com/uber/submitqueue/stovepipe/entity" "github.com/uber/submitqueue/stovepipe/extension/storage" "github.com/uber/submitqueue/test/testutil" @@ -122,7 +123,7 @@ func (s *QueueStoreContractSuite) TestQueueStore_GetNotFound() { t := s.T() _, err := s.queueStore.Get(s.ctx, "contract/does-not-exist") - assert.True(t, storage.IsNotFound(err)) + assert.ErrorIs(t, err, errs.ErrNotFound) s.log.Logf("GetNotFound passed") } @@ -149,7 +150,7 @@ func (s *QueueStoreContractSuite) TestQueueStore_UpdateCAS() { assert.Equal(t, int32(2), got.Version) err = s.queueStore.Update(s.ctx, updated, 1, 2) - assert.ErrorIs(t, err, storage.ErrVersionMismatch) + assert.ErrorIs(t, err, errs.ErrVersionMismatch) s.log.Logf("UpdateCAS passed: queue %s", name) } @@ -159,7 +160,7 @@ func (s *QueueStoreContractSuite) TestQueueStore_UpdateNotFoundIsVersionMismatch t := s.T() err := s.queueStore.Update(s.ctx, entity.Queue{Name: "contract/missing"}, 1, 2) - assert.ErrorIs(t, err, storage.ErrVersionMismatch) + assert.ErrorIs(t, err, errs.ErrVersionMismatch) s.log.Logf("UpdateNotFoundIsVersionMismatch passed") } @@ -263,7 +264,7 @@ func (s *BuildStoreContractSuite) TestBuildStore_GetNotFound() { t := s.T() _, err := s.buildStore.Get(s.ctx, "contract/does-not-exist") - assert.True(t, storage.IsNotFound(err)) + assert.ErrorIs(t, err, errs.ErrNotFound) s.log.Logf("GetNotFound passed") } @@ -291,7 +292,7 @@ func (s *BuildStoreContractSuite) TestBuildStore_UpdateCAS() { assert.Equal(t, int32(2), got.Version) err = s.buildStore.Update(s.ctx, updated, 1, 2) - assert.ErrorIs(t, err, storage.ErrVersionMismatch) + assert.ErrorIs(t, err, errs.ErrVersionMismatch) s.log.Logf("UpdateCAS passed: build %s", id) } @@ -301,7 +302,7 @@ func (s *BuildStoreContractSuite) TestBuildStore_UpdateNotFoundIsVersionMismatch t := s.T() err := s.buildStore.Update(s.ctx, entity.Build{ID: "contract/missing"}, 1, 2) - assert.ErrorIs(t, err, storage.ErrVersionMismatch) + assert.ErrorIs(t, err, errs.ErrVersionMismatch) s.log.Logf("UpdateNotFoundIsVersionMismatch passed") } diff --git a/test/integration/submitqueue/extension/storage/BUILD.bazel b/test/integration/submitqueue/extension/storage/BUILD.bazel index 7585d20d..c7256fb4 100644 --- a/test/integration/submitqueue/extension/storage/BUILD.bazel +++ b/test/integration/submitqueue/extension/storage/BUILD.bazel @@ -8,6 +8,7 @@ go_library( deps = [ "//platform/base/change:go_default_library", "//platform/base/mergestrategy:go_default_library", + "//platform/errs:go_default_library", "//submitqueue/entity:go_default_library", "//submitqueue/extension/storage:go_default_library", "//test/testutil:go_default_library", diff --git a/test/integration/submitqueue/extension/storage/suite.go b/test/integration/submitqueue/extension/storage/suite.go index dd4d0b03..af4b6f41 100644 --- a/test/integration/submitqueue/extension/storage/suite.go +++ b/test/integration/submitqueue/extension/storage/suite.go @@ -23,6 +23,7 @@ import ( "github.com/stretchr/testify/suite" "github.com/uber/submitqueue/platform/base/change" "github.com/uber/submitqueue/platform/base/mergestrategy" + "github.com/uber/submitqueue/platform/errs" "github.com/uber/submitqueue/submitqueue/entity" "github.com/uber/submitqueue/submitqueue/extension/storage" "github.com/uber/submitqueue/test/testutil" @@ -182,7 +183,7 @@ func (s *StorageContractSuite) TestStorage_OptimisticLocking() { // Try to update with stale version (should fail) err = s.storage.GetRequestStore().UpdateState(ctx, request.ID, 1, 2, entity.RequestStateLanded) assert.Error(t, err, "update with stale version should fail") - assert.ErrorIs(t, err, storage.ErrVersionMismatch, "should return ErrVersionMismatch") + assert.ErrorIs(t, err, errs.ErrVersionMismatch, "should return ErrVersionMismatch") // Verify state wasn't changed by stale update retrieved, err := s.storage.GetRequestStore().Get(ctx, request.ID) @@ -201,7 +202,7 @@ func (s *StorageContractSuite) TestStorage_NotFound() { // Try to get non-existent request _, err := s.storage.GetRequestStore().Get(ctx, "test/nonexistent") assert.Error(t, err, "getting non-existent request should return error") - assert.ErrorIs(t, err, storage.ErrNotFound, "should return ErrNotFound") + assert.ErrorIs(t, err, errs.ErrNotFound, "should return ErrNotFound") s.log.Logf("NotFound test passed: correctly returned ErrNotFound") } @@ -422,7 +423,7 @@ func (s *StorageContractSuite) TestStorage_RequestSummaryCreateGetAndCAS() { assert.NotNil(t, got.ChangeURIs) assert.NotNil(t, got.Metadata) _, err = s.storage.GetRequestSummaryStore().Get(ctx, "summary/missing") - require.ErrorIs(t, err, storage.ErrNotFound) + require.ErrorIs(t, err, errs.ErrNotFound) got.Status = entity.RequestStatusLanded got.RequestVersion = 2 @@ -430,7 +431,7 @@ func (s *StorageContractSuite) TestStorage_RequestSummaryCreateGetAndCAS() { got.LastError = "terminal detail" got.Metadata = map[string]string{"source": "test"} require.NoError(t, s.storage.GetRequestSummaryStore().Update(ctx, got, 1, 2)) - require.ErrorIs(t, s.storage.GetRequestSummaryStore().Update(ctx, got, 1, 3), storage.ErrVersionMismatch) + require.ErrorIs(t, s.storage.GetRequestSummaryStore().Update(ctx, got, 1, 3), errs.ErrVersionMismatch) updated, err := s.storage.GetRequestSummaryStore().Get(ctx, summary.RequestID) require.NoError(t, err) @@ -459,13 +460,13 @@ func (s *StorageContractSuite) TestStorage_RequestQueueSummaryListAndCursor() { assert.NotNil(t, got.ChangeURIs) assert.NotNil(t, got.Metadata) _, err = store.Get(ctx, "queue-summary", 999, "queue-summary/missing") - require.ErrorIs(t, err, storage.ErrNotFound) + require.ErrorIs(t, err, errs.ErrNotFound) got.Status = entity.RequestStatusLanded got.LastError = "done" got.Metadata = map[string]string{"result": "landed"} require.NoError(t, store.Update(ctx, got, 1, 2)) - require.ErrorIs(t, store.Update(ctx, got, 1, 3), storage.ErrVersionMismatch) + require.ErrorIs(t, store.Update(ctx, got, 1, 3), errs.ErrVersionMismatch) updated, err := store.Get(ctx, got.Queue, got.ReceivedAtMs, got.RequestID) require.NoError(t, err) assert.Equal(t, int32(2), updated.Version)