refactor(logging): Do not double log and do not classify errors#134
Merged
Conversation
behinddwalls
approved these changes
Mar 10, 2026
behinddwalls
added a commit
that referenced
this pull request
May 21, 2026
## Summary Stacked on top of #152 (change store extension). Detects duplicate land requests by reading per-URI claims from the new change store. The `start` controller writes a claim row for each URI on the request; `validate` reads the change store and rejects requests whose URIs overlap with another in-flight request in the same queue. - **start.go** — after persisting the request, claim each URI in the change store. `INSERT IGNORE` makes this idempotent on queue redelivery (same `request_id` retry is a no-op). - **validate.go** — query the change store for any row with an overlapping URI in the same queue (excluding self). For each unique candidate `request_id` returned, look up the request and skip terminal/orphan owners. If any live owner remains, reject as a user error naming the conflicting `request_id`. - Two single-table queries per check, no cross-store SQL joins. - Stop wrapping every storage error as `Retryable`; default is plain `fmt.Errorf` per the `core/errs` framework + #134. - Wires `changeStore` into both controllers in `example/server/orchestrator/main.go`. ## Behavior - **Overlap (not full-set equality):** the change store returns rows for any URI in the new request that matches a row from another in-flight request, so partial-overlap is rejected — addresses @sbalabanov's review comment. - **Liveness:** terminal-state owners and orphans (whose request row is gone) are skipped, so stale claims don't block new requests indefinitely. - **Retries:** `start` tolerates `ErrAlreadyExists` from request creation as a same-id replay; the change store's `INSERT IGNORE` is idempotent for the same reason. ## Test Plan - Unit tests for `start` and `validate` cover happy path, overlap-with-live, overlap-with-terminal-skipped, overlap-with-orphan-skipped, multi-URI-same-owner deduped, and infra-error propagation. - Existing integration tests on `extension/storage/mysql` continue to pass after `GetActiveByQueue` removal. - Change store integration test (in #152) exercises the MySQL impl against a real DB. ## Issues --------- Co-authored-by: Preetam Dwivedi <preetam@uber.com> Co-authored-by: Preetam Dwivedi <behinddwalls@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
errs.NewRetryableError, we'll do classification later and the retried errors will be "opt in" by type, rather than everything.