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
Add option to respect the marshaller specified in gRPC MethodDescriptor #5630
Conversation
grpc/src/main/java/com/linecorp/armeria/client/grpc/GrpcClientBuilder.java
Outdated
Show resolved
Hide resolved
grpc/src/main/java/com/linecorp/armeria/client/grpc/GrpcClientOptions.java
Outdated
Show resolved
Hide resolved
grpc/src/main/java/com/linecorp/armeria/client/grpc/GrpcClientBuilder.java
Outdated
Show resolved
Hide resolved
…Builder.java Co-authored-by: Trustin Lee <[email protected]>
…Options.java Co-authored-by: Trustin Lee <[email protected]>
final TestServiceBlockingStub client = | ||
GrpcClients.builder("http://foo.com") | ||
.useMethodMarshaller(true) | ||
.build(TestServiceBlockingStub.class); |
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.
Should we add a custom method Marshaller
to ensure useMethodMarshaller()
works?
We may override the method marshallers in ClientInterceptor
.
class MyClientInterceptor implements ClientInterceptor {
@Override
public <I, O> ClientCall<I, O> interceptCall(
MethodDescriptor<I, O> method, CallOptions callOptions, Channel next) {
method.toBuilder().setRequestMarshaller(new Marshaller<I>() {
@Override
public InputStream stream(I value) {
// Add a counter to ensure this method is invoked.
// Then delegate to method.requestMashaller.stream()
}
}
}
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.
I have checked to ensure marshaller
in MethodDescriptor
is working when true for useMethodMarshaller is passed through its constructor like below.
armeria/grpc/src/test/java/com/linecorp/armeria/internal/common/grpc/GrpcMessageMarshallerTest.java
Lines 77 to 86 in e11b59d
true); | |
} | |
private static Stream<Arguments> messageMarshallerArgsWithMethodMarshaller() { | |
return grpcJsonMarshallerStream().map(GrpcMessageMarshallerTest::messageMarshallerWithMethodMarshaller) | |
.map(Arguments::of); | |
} | |
@ParameterizedTest | |
@MethodSource({"messageMarshallerArgs", "messageMarshallerArgsWithMethodMarshaller"}) |
If it's not enough, I'll try to add testcast.
What do you think?
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.
Applied feedback.
Thanks for your recommendation ClientInterceptor
. It works fine.
.useMethodMarshaller(true) | ||
.build(TestServiceBlockingStub.class); | ||
|
||
stub.unaryCall(SimpleRequest.getDefaultInstance()); | ||
assertThat(customMarshallerInterceptor.getSpiedMarshallerCallCnt()).isEqualTo(1); |
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.
Should we test with useMethodMarshaller(false)
? We may use @ParameterizedTest
@ParameterizedTest
@CsvResouce({"true, 1", "false, 0"})
void useMethodMarshaller(boolean useMethodMarshaller, int count) {
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.
Thanks for feedback.
I've found that spy-intercepter had been called even though setting useMethodMarshaller
to false
.
I don't have any idea at this moment, but It seems to need modification of testcase.
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.
I realized we needed to implement PrototypeMarshaller
for the mock mashaller.
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.
Thank you, It(PrototypeMarshaller
) works.
grpc/src/test/java/com/linecorp/armeria/server/grpc/UnaryServerCallTest.java
Outdated
Show resolved
Hide resolved
grpc/src/test/java/com/linecorp/armeria/client/grpc/GrpcClientBuilderTest.java
Outdated
Show resolved
Hide resolved
…rCallTest.java Co-authored-by: Ikhun Um <[email protected]>
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.
Almost there!
} | ||
}; | ||
|
||
private static class CustomMarshallerInterceptor implements ClientInterceptor { |
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.
Code style) Could we place this class at the bottom of this file so that we can see the test cases first when we open this file?
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.
Applied feedback. Thanks.
@@ -260,6 +276,90 @@ void enableHttpJsonTranscodingWithoutJsonSupport() { | |||
.hasMessageContaining("enableHttpJsonTranscoding"); | |||
} | |||
|
|||
static int spiedMarshallerCallCnt; |
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.
Should we remove this name and use a local variable instead? I don't see this field is shared with others.
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.
Applied feedback. Thanks.
|
||
@Override | ||
public InputStream stream(SimpleRequest value) { | ||
spiedMarshallerCallCnt++; |
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.
Should we have two different counters instead of the unified one in case one method is called twice.
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.
Applied feedback. Thanks.
… variable for both request and response. Add additional description for requestStreamCallCnt
CustomMarshallerInterceptor() { | ||
} | ||
|
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.
It looks redundant.
CustomMarshallerInterceptor() { | |
} |
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.
Applied feedback, Thanks.
final AtomicInteger requestStreamCallCnt = new AtomicInteger(0); | ||
final AtomicInteger responseStreamCallCnt = new AtomicInteger(0); |
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.
nit: 0
is the default value if not set.
final AtomicInteger requestStreamCallCnt = new AtomicInteger(0); | |
final AtomicInteger responseStreamCallCnt = new AtomicInteger(0); | |
final AtomicInteger requestStreamCallCnt = new AtomicInteger(); | |
final AtomicInteger responseStreamCallCnt = new AtomicInteger(); |
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.
Applied feedback, Thanks.
grpc/src/main/java/com/linecorp/armeria/internal/common/grpc/GrpcMessageMarshaller.java
Show resolved
Hide resolved
…odMarshaller' mutually exclusive state
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.
Just some nits. Well done, @jaeseung-bae!
grpc/src/main/java/com/linecorp/armeria/client/grpc/GrpcClientBuilder.java
Outdated
Show resolved
Hide resolved
grpc/src/main/java/com/linecorp/armeria/server/grpc/GrpcServiceBuilder.java
Outdated
Show resolved
Hide resolved
grpc/src/test/java/com/linecorp/armeria/client/grpc/GrpcClientBuilderTest.java
Outdated
Show resolved
Hide resolved
dabd601
to
7d25d00
Compare
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.
Left a minor comment on testing but looks good! Thanks @jaeseung-bae 🙇 👍 🙇
}; | ||
|
||
final TestServiceImplBase testService = new TestServiceImpl( | ||
Executors.newSingleThreadScheduledExecutor()); |
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.
Executors.newSingleThreadScheduledExecutor()); | |
CommonPools.blockingTaskExecutor()); |
grpc/src/test/java/com/linecorp/armeria/server/grpc/GrpcServiceBuilderTest.java
Outdated
Show resolved
Hide resolved
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.
Basically, looks great! 👍 Left some suggestions. 😉
@@ -399,6 +411,12 @@ public <T> T build(Class<T> clientType) { | |||
|
|||
final Object client; | |||
final ClientOptions options = buildOptions(); | |||
if (options.get(GrpcClientOptions.UNSAFE_WRAP_RESPONSE_BUFFERS) && options.get(USE_METHOD_MARSHALLER)) { |
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.
Could you move logic into enableUnsafeWrapResponseBuffers
and useMethodMarshaller
methods so that the exception is thrown early when the property is set?
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.
Okay, I'll move logic to both enableUnsafeWrapResponseBuffers()
and useMethodMarshaller()
method.
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.
Applied feedbacks, Thanks.
grpc/src/main/java/com/linecorp/armeria/internal/client/grpc/GrpcClientFactory.java
Outdated
Show resolved
Hide resolved
grpc/src/main/java/com/linecorp/armeria/internal/common/grpc/GrpcMessageMarshaller.java
Outdated
Show resolved
Hide resolved
grpc/src/main/java/com/linecorp/armeria/server/grpc/GrpcServiceBuilder.java
Outdated
Show resolved
Hide resolved
grpc/src/main/java/com/linecorp/armeria/internal/common/grpc/GrpcMessageMarshaller.java
Outdated
Show resolved
Hide resolved
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.
Excellent work, @jaeseung-bae! 👍 👍 👍
Thanks!
@jaeseung-bae 👍 👍 👍 |
Motivation:
MethodDescriptor
MethodDescriptor
Modifications:
useMethodMarshaller
false
GrpcServiceBuilder
andGrpcClientBuilder
to check thatunsafeWrapDeserializedBuffer
anduseMethodMarshaller
are mutually exclusiveResult:
useMethodMarshaller
IllegalStateException
when bothunsafeWrapDeserializedBuffer
anduseMethodMarshaller
are enabled