-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Donot return dgo.ErrAborted when client calls txn.Discard() #4389
Conversation
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.
Tested against these sample codes again.
The previously reported issue doesn't seem to exist anymore in any of the 3 clients.
Reviewed 2 of 2 files at r1.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @animesh2049, @mangalaman93, and @manishrjain)
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.
apart from a couple of minor comments.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @animesh2049, @ashish-goswami, @mangalaman93, and @manishrjain)
contrib/integration/testtxn/main_test.go, line 944 at r1 (raw file):
err = txn.Discard(context.Background()) // Since client is discarding this transaction server should not throw ErrAborted err.
so what's the value of err? if it's supposed to be nil you could say
require.NoError(t, err)
instead.
edgraph/server.go, line 876 at r1 (raw file):
means client
means the client
edgraph/server.go, line 877 at r1 (raw file):
Hence we should not return this error.
change to "Return a nil error instead" to make it clearer the call is meant to be marked as successful.
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.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @animesh2049, @mangalaman93, @manishrjain, and @martinmr)
contrib/integration/testtxn/main_test.go, line 944 at r1 (raw file):
Previously, martinmr (Martin Martinez Rivera) wrote…
so what's the value of err? if it's supposed to be nil you could say
require.NoError(t, err)
instead.
I am not very sure if any other error can be returned while calling txn.Discard()
. If some error is returned it should not be equal to dgo.ErrAborted
.
edgraph/server.go, line 876 at r1 (raw file):
Previously, martinmr (Martin Martinez Rivera) wrote…
means client
means the client
Done.
edgraph/server.go, line 877 at r1 (raw file):
Previously, martinmr (Martin Martinez Rivera) wrote…
Hence we should not return this error.
change to "Return a nil error instead" to make it clearer the call is meant to be marked as successful.
Done.
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.
Reviewed 1 of 2 files at r1, 1 of 1 files at r2.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @animesh2049, @mangalaman93, and @martinmr)
Related to dgraph-io/dgraph-js#48
This change is