-
Notifications
You must be signed in to change notification settings - Fork 6
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
fix: add message wrapper to errors and improve resource exhausted error message by using metadata #551
Conversation
…or message by using metadata
@@ -30,7 +31,7 @@ func errUnexpectedGrpcResponse(r requester, grpcResp grpcResponse) momentoerrors | |||
type requester interface { | |||
hasCacheName | |||
initGrpcRequest(client scsDataClient) error | |||
makeGrpcRequest(metadata context.Context, client scsDataClient) (grpcResponse, error) | |||
makeGrpcRequest(requestMetadata context.Context, client scsDataClient) (grpcResponse, []metadata.MD, error) |
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.
had to update this signature to be able to get the response metadata in the cache data client.
a lot of the changes are to make the requests conform to this change.
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.
This is a comprehensive change.
Suggestions for improvement:
- Centralize all static strings to one place
- Extract any complicated error logic to a function instead of directly in a switch
This way we can write unit tests for various more complicated things, and maintain uniformity over the messages.
…apper
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.
One last question otherwise good to go.
Closes https://github.com/momentohq/dev-eco-issue-tracker/issues/1038
err
metadata in order to report the cause in the message wrapper. If the metadata is not available, we try to use string matching on the error details.AlreadyExistsError
in favor ofCacheAlreadyExistsError
andStoreAlreadyExistsError
for consistency with the other SDKs.