Skip to content
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

feat: refactor error to be more clear and maintainable #226

Merged
merged 8 commits into from
Feb 19, 2024

Conversation

PureWhiteWu
Copy link
Member

@PureWhiteWu PureWhiteWu commented Feb 18, 2024

  1. All the thrift exceptions should be defined together, so this pr moves ApplicationError from volo to pilota.
  2. The previous XError such as ApplicationError, ProtocolError, TransportError correspond to the exception concept in thrift rpc protocol, this pr renames those types to XException.
  3. The Error type does not have a clear semantic, this pr renames it to ThriftException which is clearer in meaning.
  4. The write_x methods in rw_ext.rs never returns error, so this PR removes the Result type from function sig.
  5. Previously the TransportException only comes from std::io::Error and its kind is a subset of std::io::ErrorKind, and separate a new TransportExceptionKind from std::io::ErrorKind does not make sense. So this pr deletes the TransportExceptionKind and uses the std::io::Error directly.
  6. Previously there's a standalone type called DecodeError which is a mixture of some variants of ThriftException, and the reason it exists is to add more information about the context of the error. But the implementation has some issues, such as we cannot easily get the root kind of error. The only thing we need to do is to prepend a message to the error, so this pr adds a prepend_msg function to ThriftException and uses it as the codec error, which is far clearer and more maintainable.

@PureWhiteWu PureWhiteWu added A-pilota This issue concerns the main `pilota` crate. A-pilota-build This issue concerns the `pilota-build` crate. labels Feb 18, 2024
@PureWhiteWu PureWhiteWu self-assigned this Feb 18, 2024
@PureWhiteWu PureWhiteWu requested review from a team as code owners February 18, 2024 07:24
@GuangmingLuo

This comment was marked as resolved.

@PureWhiteWu

This comment was marked as resolved.

pilota-build/test_data/thrift/default_value.rs.bak Outdated Show resolved Hide resolved
pilota-build/test_data/thrift/normal.rs.bak Outdated Show resolved Hide resolved
pilota/src/thrift/error/protocol.rs Outdated Show resolved Hide resolved
pilota/src/thrift/error/protocol.rs Show resolved Hide resolved
pilota/src/thrift/error/application.rs Outdated Show resolved Hide resolved
pilota/src/thrift/error/application.rs Show resolved Hide resolved
pilota/src/thrift/error/application.rs Outdated Show resolved Hide resolved
pilota/src/thrift/error/transport.rs Show resolved Hide resolved
@PureWhiteWu PureWhiteWu merged commit 9bc9ae3 into cloudwego:main Feb 19, 2024
9 checks passed
@PureWhiteWu PureWhiteWu deleted the refactor/error branch February 19, 2024 08:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-pilota This issue concerns the main `pilota` crate. A-pilota-build This issue concerns the `pilota-build` crate.
Development

Successfully merging this pull request may close these issues.

4 participants