Skip to content

Conversation

@biosvs
Copy link
Collaborator

@biosvs biosvs commented Mar 15, 2024

Functions newClientEncodingError and newServerEncodingError are called only when encoding/decoding attempt is failed, which means that either structure can't be transformed into bytes, or bytes can't be read into structure.

Currently such errors lead to CodeInvalidArgument (3) return code.

As yarpc error codes relates to grpc error codes, we may assume that meaning is also close (if not the same). In grpc-go source code the same encoding/decoding errors lead to CodeInternal code (1, 2), and so the same code should be returned in yarpc.

This PR changes CodeInvalidArgument (3) to CodeInternal(13) for most such cases.

Except for server decoding errors. If server can't decode body or header of the request, it indeed indicates that client tries to send something unexpected. (This is a change from previous review.)

It's an API change, but in a worst case we expect to have more visibility on problems that were hidden by "client errors".

RELEASE NOTES:

  • errors: encoding/decoding (marshalling/unmarshalling) errors are now returned as CodeInternal (13) instead of CodeInvalidArgument (3).

AllenLuUber
AllenLuUber previously approved these changes Mar 19, 2024
@AllenLuUber AllenLuUber changed the title Changed encoding/decoding error code Changed encoding/decoding error code to from InvalidArgument to Internal Mar 19, 2024
@biosvs biosvs force-pushed the change-enc-dec-error-code branch from 7189d58 to 9f028c7 Compare March 19, 2024 17:15
@biosvs biosvs force-pushed the change-enc-dec-error-code branch from 9f028c7 to 4a35a1d Compare April 30, 2024 13:24
@biosvs biosvs force-pushed the change-enc-dec-error-code branch from 4a35a1d to f0ff598 Compare April 30, 2024 14:27
@biosvs biosvs requested a review from AllenLuUber April 30, 2024 14:28
@biosvs
Copy link
Collaborator Author

biosvs commented Apr 30, 2024

I revisited code and tests, and revert changes for server decoding: those errors indeed indicates that client sends invalid request.

@codecov
Copy link

codecov bot commented Apr 30, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.67%. Comparing base (7df43bc) to head (d065083).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2251      +/-   ##
==========================================
- Coverage   84.69%   84.67%   -0.03%     
==========================================
  Files         276      276              
  Lines       16102    16104       +2     
==========================================
- Hits        13638    13636       -2     
- Misses       2019     2021       +2     
- Partials      445      447       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@biosvs biosvs added this to the v1.73.3 milestone Sep 24, 2024
@biosvs biosvs modified the milestones: v1.75.0, v1.75.4 Dec 11, 2024
@biosvs biosvs changed the title Changed encoding/decoding error code to from InvalidArgument to Internal Changed encoding/decoding error code from InvalidArgument to Internal Dec 12, 2024
@biosvs biosvs removed this from the v1.75.4 milestone Jan 29, 2025
Signed-off-by: Vitalii Levitskii <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants