Should we set DefaultGrpcExceptionHandlerFunction as a fallback exception handler for exceptionHandler when it is set in the GrpcClientBuilder like we do in ServerBuilder?

Okay, I'll modify to use DefaultGrpcExceptionHandlerFunction as fallback exception handler.

Global comments) If DefaultGrpcExceptionHandlerFunction is set as the fallback, should we remove DefaultGrpcExceptionHandlerFunction here and others?

Ok, I'll

Applied feedback. Thanks

reportStatus is unrelated to GrpcExceptionHandlerFunction. Should we leave it in GrpcStatus and only move code handling exceptions to DefaultGrpcExceptionHandlerFunction?

Thanks for the recommendation. I also think we should separate GrpcStatus and DefaultGrpcExceptionHandlerFunction.

Applied feedback. Thanks

Let's expose DefaultGrpcExceptionHandlerFunction using GrpcExceptionHandlerFunction.ofDefault().

Ok, I'll.

Applied feedback. Thanks

Global comments) If DefaultGrpcExceptionHandlerFunction is set as the fallback, should we remove DefaultGrpcExceptionHandlerFunction here and others?

We don't have to use DefaultGrpcExceptionHandlerFunction to call the exceptionHandler.
If DefaultGrpcExceptionHandlerFunction is necessary, exceptionHandler.orElse(GrpcExceptionHandlerFunction.ofDefault() can be used.

Applied feedback. Thanks

isSameAs() is probably a better choice in this case because we want it to be an exactly same instance.

This testcase has been removed after some modification.
But there is still test case useDefaultGrpcExceptionHandlerFunctionAsFallback

I believe that we shouldn't set the property while building an instance:

GrpcExceptionHandlerFunction exceptionHandler;
if (this.exceptionHandler == null) {
    exceptionHandler = GrpcExceptionHandlerFunction.ofDefault();
} else {
    exceptionHandler = this.exceptionHandler.orElse(GrpcExceptionHandlerFunction.ofDefault());
}
exceptionHandler = new UnwrappingGrpcExceptionHandleFunction(exceptionHandler);
option(EXCEPTION_HANDLER.newValue(exceptionHandler));

Currently, exceptionHandler is passed through ClientBuilderParams params for instantiating ArmeriaChannel. This look like a current way to pass parameters by ClientBuilderParams and so exceptionHandler should be set. I may introduce exceptionHandler as new constructor parameter but I'm not sure that is right to do. Do you have any idea on this?

The exceptionHandler is retrieved from options so we don't need to add exceptionHandler to the constructor parameter:

I also realized that we have to create a new exceptionHandler only when it's set:

public <T> T build(Class<T> clientType) {
    requireNonNull(clientType, "clientType");

    final List<ClientInterceptor> clientInterceptors = interceptors.build();
    if (!clientInterceptors.isEmpty()) {
        option(INTERCEPTORS.newValue(clientInterceptors));
    }
    if (exceptionHandler != null) {
        option(EXCEPTION_HANDLER.newValue(new UnwrappingGrpcExceptionHandleFunction(
                exceptionHandler.orElse(GrpcExceptionHandlerFunction.of()))));
    }
    ....
}

public static final ClientOption<GrpcExceptionHandlerFunction> EXCEPTION_HANDLER =
        ClientOption.define("EXCEPTION_HANDLER", new UnwrappingGrpcExceptionHandleFunction(
                GrpcExceptionHandlerFunction.of()));
               

We don't use UnwrappingGrpcExceptionHandleFunction because this value is never used. Is that correct?

UnwrappingGrpcExceptionHandleFunction should be the outermost handler so we don't use it here.

Then, probably need to add a comment for that?
GrpcExceptionHandlerFunction.ofDefault() isn't used because the actual exceptionHandler is set while building a gRPC client in GrpcClientBuilder.

Added comment, @minwoox can you check it again? Thanks,

Based on this change, we have to use it now.
Sorry but could you revert it? 😓

Okay, I'll revert it

Recently, we just use of for the default instance.

Applied feedback, Thanks.

nit:

Applied feedback, Thanks.

nit:

Applied feedback, Thanks.

ditto

Is this ditto to #5571 (comment)?
I replied to #5571 (comment)?.

Yes we can probably do:

GrpcExceptionHandlerFunction grpcExceptionHandler;
if (exceptionMappingsBuilder != null) {
    grpcExceptionHandler = exceptionMappingsBuilder.build().orElse(GrpcExceptionHandlerFunction.of());
} else if (exceptionHandler != null) {
    grpcExceptionHandler = exceptionHandler.orElse(GrpcExceptionHandlerFunction.of());;
} else {
    grpcExceptionHandler = GrpcExceptionHandlerFunction.of();
}
grpcExceptionHandler = new UnwrappingGrpcExceptionHandleFunction(grpcExceptionHandler);
registryBuilder.setDefaultExceptionHandler(grpcExceptionHandler);

if (interceptors != null) {
    final HandlerRegistry.Builder newRegistryBuilder = new HandlerRegistry.Builder();
    final ImmutableList<ServerInterceptor> interceptors = this.interceptors.build();
    for (Entry entry : registryBuilder.entries()) {
        final MethodDescriptor<?, ?> methodDescriptor = entry.method();
        final ServerServiceDefinition intercepted =
                ServerInterceptors.intercept(entry.service(), interceptors);
        newRegistryBuilder.addService(entry.path(), intercepted, methodDescriptor, entry.type(),
                                      entry.additionalDecorators());
    }
    newRegistryBuilder.setDefaultExceptionHandler(grpcExceptionHandler);
    handlerRegistry = newRegistryBuilder.build();
} else {
    handlerRegistry = registryBuilder.build();
}

Applied feedback, Thank.
Code gets cleaner :)