-
Notifications
You must be signed in to change notification settings - Fork 1.6k
feat(txn): surface transaction abort reasons to clients #9747
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
rahst12
wants to merge
9
commits into
dgraph-io:main
Choose a base branch
from
rahst12:txn-abort-reason-surface-phase-1
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 2 commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
11ff9ec
first draft of surfacing the reason transaction abort/error occured
rahst12 1042bb1
adding test
rahst12 0f2c0de
Merge remote-tracking branch 'origin/main' into txn-abort-reason-surf…
rahst12 59855c9
fix(txn): report the cause an abort actually had, not the one nearby
rahst12 29d86d0
refactor(txn): declare every abort detail in one block
rahst12 b3d40ad
feat(txn): name the key kinds a conflict abort cannot distinguish
rahst12 d1dddc7
fix(txn): report aborts that happened before the commit was decided
rahst12 c7f2f35
fix(zero): parse group id with ParseUint so the uint32 conversion can…
rahst12 71961d7
Merge branch 'main' into txn-abort-reason-surface-phase-1
rahst12 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,63 @@ | ||
| /* | ||
| * SPDX-FileCopyrightText: © 2017-2025 Istari Digital, Inc. | ||
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
|
|
||
| package zero | ||
|
|
||
| import ( | ||
| "strings" | ||
| "testing" | ||
|
|
||
| "github.com/stretchr/testify/require" | ||
|
|
||
| "github.com/dgraph-io/dgo/v250/protos/api" | ||
| ) | ||
|
|
||
| // The abort-reason wire format is a contract with gRPC clients (e.g. dgraph4j parses the | ||
| // "<code>: " prefix into TxnConflictException.AbortReason). These unit tests pin the | ||
| // category prefixes and the logic that selects between them, so the contract can't drift | ||
| // silently without an integration cluster. | ||
|
|
||
| func TestAbortReasonFormat(t *testing.T) { | ||
| require.Equal(t, "conflict: boom", abortReason(abortReasonConflict, "boom")) | ||
| require.Equal(t, "stale-startts: x", abortReason(abortReasonStaleStartTs, "x")) | ||
| require.Equal(t, "predicate-move: y", abortReason(abortReasonPredicateMove, "y")) | ||
| } | ||
|
|
||
| func TestConflictAbortReason(t *testing.T) { | ||
| // Write-write conflict. | ||
| r := conflictAbortReason(false) | ||
| require.True(t, strings.HasPrefix(r, abortReasonConflict+": "), | ||
| "want conflict prefix, got %q", r) | ||
| require.Equal(t, abortReason(abortReasonConflict, abortDetailConflict), r) | ||
|
|
||
| // Stale start timestamp (leader change). | ||
| r = conflictAbortReason(true) | ||
| require.True(t, strings.HasPrefix(r, abortReasonStaleStartTs+": "), | ||
| "want stale-startts prefix, got %q", r) | ||
| require.Equal(t, abortReason(abortReasonStaleStartTs, abortDetailStaleStartTs), r) | ||
| require.Contains(t, r, "leader change") | ||
| } | ||
|
|
||
| // TestHasConflictStaleStartTs pins the exact discriminator commit() uses to choose the | ||
| // stale-startts reason: a txn whose startTs predates the leader's startTxnTs lease is a | ||
| // conflict, and is flagged stale; a fresh startTs with no conflicting keys is neither. | ||
| func TestHasConflictStaleStartTs(t *testing.T) { | ||
| o := &Oracle{} | ||
| o.Init() | ||
| defer o.close() | ||
|
|
||
| o.updateStartTxnTs(100) | ||
|
|
||
| // startTs below the lease floor: hasConflict true, and the stale discriminator true. | ||
| stale := &api.TxnContext{StartTs: 42} | ||
| require.True(t, o.hasConflict(stale), "txn below startTxnTs must conflict") | ||
| require.True(t, stale.StartTs < o.startTxnTs, "must be flagged stale") | ||
| require.Equal(t, conflictAbortReason(true), conflictAbortReason(stale.StartTs < o.startTxnTs)) | ||
|
|
||
| // startTs at/above the lease floor with no keys: not a conflict, not stale. | ||
| fresh := &api.TxnContext{StartTs: 100} | ||
| require.False(t, o.hasConflict(fresh), "fresh txn with no keys must not conflict") | ||
| require.False(t, fresh.StartTs < o.startTxnTs, "must not be flagged stale") | ||
| } |
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
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tctx.Aborted = truehere (and the matching line indoMutate) is inert as far as I can tell.gRPC drops the response message when a unary handler returns a non-nil error, so no gRPC client sees the flag. The in-process callers don't read it either:
handleCommit(dgraph/cmd/alpha/http.go:605) returns onerr != nilbefore touchingtc,handleAbortis the client-discard path, and bothgraphql/resolve/mutation.gocall sites discard the context on error.Harmless and consistent with the existing
dgo.ErrAbortedbranch above, so I'm not asking you to pull it — but the PR description sells "setAborted" as part of the fix and it isn't observable anywhere.