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

Support gRPC exception handler for client-side #5351

Merged
merged 4 commits into from Jan 19, 2024

Conversation

jrhee17
Copy link
Contributor

@jrhee17 jrhee17 commented Dec 19, 2023

Motivation:

This PR attempts to support GrpcExceptionHandlerFunction for gRPC client side

Modifications:

  • Add GrpcClientBuilder.exceptionHandler which accepts a GrpcExceptionHandlerFunction
  • Modify all GrpcStatus.fromThrowable(Throwable) calls to use GrpcStatus.fromThrowable(GrpcStatusFunction, RequestContext, Throwable, Metadata) instead

Result:

@jrhee17 jrhee17 added this to the 1.27.0 milestone Dec 20, 2023
@jrhee17 jrhee17 marked this pull request as ready for review December 20, 2023 01:45
Copy link
Contributor

@ikhoon ikhoon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @jrhee17! 👍

* via request metadata.
*/
public static final ClientOption<GrpcExceptionHandlerFunction> EXCEPTION_HANDLER =
ClientOption.define("EXCEPTION_HANDLER", (ctx, cause, metadata) -> null);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor) Should we provide the default exception handler with this option?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I don't think I understood your intention 😅

Do you mean you prefer that (ctx, cause, metadata) -> null is defined as the "DEFAULT" exception handler somewhere? (so that GrpcExceptionHandlerFunction is non-null overall?)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess (ctx, cause, metadata) -> null is the default exception handler which returns null so that the status from statusFromThrowable is used.

if (exceptionHandler != null) {
final Status status = exceptionHandler.apply(ctx, t, metadata);
if (status != null) {
return status;
}
}
return statusFromThrowable(t);

I think we can explicitly call (ctx, cause, metadata) -> GrpcStatus.fromThrowable(cause) or leave a comment about why null is returned to make it clearer. 🤔

Copy link
Contributor Author

@jrhee17 jrhee17 Jan 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated as suggested 👍

Copy link
Member

@minwoox minwoox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thanks! 👍 👍 👍

Comment on lines 170 to 171
* Sets the {@link CallCredentials} that carries credential data that will be propagated to the server
* via request metadata.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't revised. 😉

* via request metadata.
*/
public static final ClientOption<GrpcExceptionHandlerFunction> EXCEPTION_HANDLER =
ClientOption.define("EXCEPTION_HANDLER", (ctx, cause, metadata) -> null);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess (ctx, cause, metadata) -> null is the default exception handler which returns null so that the status from statusFromThrowable is used.

if (exceptionHandler != null) {
final Status status = exceptionHandler.apply(ctx, t, metadata);
if (status != null) {
return status;
}
}
return statusFromThrowable(t);

I think we can explicitly call (ctx, cause, metadata) -> GrpcStatus.fromThrowable(cause) or leave a comment about why null is returned to make it clearer. 🤔

Copy link

codecov bot commented Jan 11, 2024

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (3f54be0) 73.94% compared to head (7a44db6) 73.69%.
Report is 9 commits behind head on main.

Files Patch % Lines
...rmeria/internal/client/grpc/ArmeriaClientCall.java 81.81% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #5351      +/-   ##
============================================
- Coverage     73.94%   73.69%   -0.26%     
- Complexity    20104    20107       +3     
============================================
  Files          1730     1741      +11     
  Lines         74161    74369     +208     
  Branches       9465     9483      +18     
============================================
- Hits          54841    54804      -37     
- Misses        14844    15030     +186     
- Partials       4476     4535      +59     

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

@jrhee17
Copy link
Contributor Author

jrhee17 commented Jan 19, 2024

Let me go ahead and merge this 🙇 Thanks for the review all

@jrhee17 jrhee17 merged commit 9503d51 into line:main Jan 19, 2024
13 of 15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow custom gRPC status mapping rule in gRPC client
3 participants