-
Notifications
You must be signed in to change notification settings - Fork 37
Add "Aborted" servicerror #223
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
Conversation
9afcfc0
to
6e9b4ad
Compare
@@ -94,7 +94,7 @@ func FromStatus(st *status.Status) error { | |||
case *failure.MultiOperationExecutionAborted: | |||
return newMultiOperationAborted(st) | |||
default: | |||
// fall through to st.Err() | |||
return newAborted(st) |
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.
not an issue.
It looks like the only different between those errors (like return newUnavailable(st)
) is the name of the structure, and the rest is the same.
Any specific reason we choose this approach instead of "newErrorWithCode" or something like that, that looks like
ErrorWithCode struct {
Message string
Code codes.Code
st *status.Status
}
and then other "concrete" errors inherits from it? This way we may avoid reflection, and reduce number of error types.
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.
and then other "concrete" errors inherits from it? This way we may avoid reflection, and reduce number of error types.
Go doesn't have inheritance, and if embedding this like type Aborted struct { ErrorWithCode }
you aren't reducing the number of error types, and not sure what is meant by reflection here. The duplication here is fairly normal in these cases.
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.
Nothing technically blocking
@@ -94,7 +94,7 @@ func FromStatus(st *status.Status) error { | |||
case *failure.MultiOperationExecutionAborted: | |||
return newMultiOperationAborted(st) | |||
default: | |||
// fall through to st.Err() | |||
return newAborted(st) |
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.
I am (only slightly) concerned this may be technically incompatible behavior changing an error type that users may be using. Can we make sure that this is clearly called out in the release notes of the next release of this repo?
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.
Right; I also thought about that for a bit.
From what I understand, the purpose of FromStatus
is to convert status.Status
to a particular type of service error; or fallback to status.Error
if there isn't one available.
An incompatibility issue would occur if a user
(1) relied on the error message details (.Error()
) or
(2) tried to cast the result to a status.Error
Both seem unlikely, but definitely technically possible.
👍 I can add a warning to the release notes.
@@ -94,7 +94,7 @@ func FromStatus(st *status.Status) error { | |||
case *failure.MultiOperationExecutionAborted: | |||
return newMultiOperationAborted(st) | |||
default: | |||
// fall through to st.Err() | |||
return newAborted(st) |
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.
What about out of range and unauthenticated? Should we do those now too or wait?
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.
Good question. I'm not sure tbh. Doing it now batches the "breaking" changes; but we might also never change this.
Generally, I understand this method to act as "try to parse this status to a typed error so I can extract error details". If the status cannot be converted, I wouldn't expect users to use/expect the status.Error
since it doesn't add anything over status.Status
. But they still might ...
What changed?
Added
Aborted
serviceerror.Why?
There's a need to return an
Aborted
serviceerror from the server.Given that we these serviceerror types for each status code, it seems like the right thing to add this here.
How did you test it?
Potential risks