Add option to respect the marshaller specified in gRPC MethodDescriptor #5630

jaeseung-bae · 2024-04-22T14:53:48Z

Motivation:

This PR adds option to use marshaller that specified in gRPC MethodDescriptor
Related Issue Provide a way to provide custom protobuf marshallers #5103
- Partial solved: Option to use marshaller that specified in gRPC MethodDescriptor
- Unsolved part: Provide way to add custom marshaller

Modifications:

New option useMethodMarshaller
- default value is false
Add validate logic for GrpcServiceBuilder and GrpcClientBuilder to check that unsafeWrapDeserializedBuffer and useMethodMarshaller are mutually exclusive

Result:

Have new option useMethodMarshaller
Throw IllegalStateException when both unsafeWrapDeserializedBuffer and useMethodMarshaller are enabled

…scriptor

github-actions · 2024-04-22T16:11:37Z

🔍 Build Scan® (commit: `33b67a4`)

Job name	Status	Build Scan®
build-windows-latest-jdk-21	✅	https://ge.armeria.dev/s/c3t44iglkfbhy
build-self-hosted-unsafe-jdk-8	✅	https://ge.armeria.dev/s/4r3nstqem5kew
build-self-hosted-unsafe-jdk-21-snapshot-blockhound	✅	https://ge.armeria.dev/s/gkmvjbkvtpeyi
build-self-hosted-unsafe-jdk-17-min-java-17-coverage	✅	https://ge.armeria.dev/s/ixyeas4apsboo
build-self-hosted-unsafe-jdk-17-min-java-11	✅	https://ge.armeria.dev/s/dfrsbi6sxrzok
build-self-hosted-unsafe-jdk-17-leak	✅	https://ge.armeria.dev/s/i3ke73bcj5x72
build-self-hosted-unsafe-jdk-11	✅	https://ge.armeria.dev/s/kwk5fwzm2am4o
build-macos-12-jdk-21	✅	https://ge.armeria.dev/s/zagkltchhibgs

grpc/src/main/java/com/linecorp/armeria/client/grpc/GrpcClientBuilder.java

grpc/src/main/java/com/linecorp/armeria/client/grpc/GrpcClientOptions.java

grpc/src/main/java/com/linecorp/armeria/client/grpc/GrpcClientBuilder.java

…Builder.java Co-authored-by: Trustin Lee <t@motd.kr>

…Options.java Co-authored-by: Trustin Lee <t@motd.kr>

…rectly

ikhoon · 2024-04-24T06:09:22Z

grpc/src/test/java/com/linecorp/armeria/client/grpc/GrpcClientBuilderTest.java

+        final TestServiceBlockingStub client =
+                GrpcClients.builder("http://foo.com")
+                           .useMethodMarshaller(true)
+                           .build(TestServiceBlockingStub.class);


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() } } }

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?

Applied feedback.
Thanks for your recommendation ClientInterceptor. It works fine.

…correctly

ikhoon · 2024-04-25T07:15:50Z

grpc/src/test/java/com/linecorp/armeria/client/grpc/GrpcClientBuilderTest.java

+                                                        .useMethodMarshaller(true)
+                                                        .build(TestServiceBlockingStub.class);
+
+        stub.unaryCall(SimpleRequest.getDefaultInstance());
+        assertThat(customMarshallerInterceptor.getSpiedMarshallerCallCnt()).isEqualTo(1);


Should we test with useMethodMarshaller(false)? We may use @ParameterizedTest

@ParameterizedTest @CsvResouce({"true, 1", "false, 0"}) void useMethodMarshaller(boolean useMethodMarshaller, int count) {

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.

I realized we needed to implement PrototypeMarshaller for the mock mashaller.

Thank you, It(PrototypeMarshaller) works.

grpc/src/test/java/com/linecorp/armeria/server/grpc/UnaryServerCallTest.java

grpc/src/test/java/com/linecorp/armeria/client/grpc/GrpcClientBuilderTest.java

…rCallTest.java Co-authored-by: Ikhun Um <ih.pert@gmail.com>

…correctly

ikhoon

Almost there!

ikhoon · 2024-04-26T10:51:42Z

grpc/src/test/java/com/linecorp/armeria/client/grpc/GrpcClientBuilderTest.java

+        }
+    };
+
+    private static class CustomMarshallerInterceptor implements ClientInterceptor {


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?

Applied feedback. Thanks.

ikhoon · 2024-04-26T10:55:23Z

grpc/src/test/java/com/linecorp/armeria/server/grpc/GrpcServiceBuilderTest.java

@@ -260,6 +276,90 @@ void enableHttpJsonTranscodingWithoutJsonSupport() {
                .hasMessageContaining("enableHttpJsonTranscoding");
    }

+    static int spiedMarshallerCallCnt;


Should we remove this name and use a local variable instead? I don't see this field is shared with others.

Applied feedback. Thanks.

ikhoon · 2024-04-26T10:57:03Z

grpc/src/test/java/com/linecorp/armeria/server/grpc/GrpcServiceBuilderTest.java

+
+            @Override
+            public InputStream stream(SimpleRequest value) {
+                spiedMarshallerCallCnt++;


Should we have two different counters instead of the unified one in case one method is called twice.

Applied feedback. Thanks.

… well

… variable for both request and response. Add additional description for requestStreamCallCnt

ikhoon · 2024-04-29T07:32:05Z

grpc/src/test/java/com/linecorp/armeria/client/grpc/GrpcClientBuilderTest.java

+        CustomMarshallerInterceptor() {
+        }
+


It looks redundant.

Suggested change

CustomMarshallerInterceptor() {

}

Applied feedback, Thanks.

ikhoon · 2024-04-29T07:34:07Z

grpc/src/test/java/com/linecorp/armeria/server/grpc/GrpcServiceBuilderTest.java

+        final AtomicInteger requestStreamCallCnt = new AtomicInteger(0);
+        final AtomicInteger responseStreamCallCnt = new AtomicInteger(0);


nit: 0 is the default value if not set.

Suggested change

final AtomicInteger requestStreamCallCnt = new AtomicInteger(0);

final AtomicInteger responseStreamCallCnt = new AtomicInteger(0);

final AtomicInteger requestStreamCallCnt = new AtomicInteger();

final AtomicInteger responseStreamCallCnt = new AtomicInteger();

Applied feedback, Thanks.

grpc/src/main/java/com/linecorp/armeria/internal/common/grpc/GrpcMessageMarshaller.java

…odMarshaller' mutually exclusive state

trustin

Just some nits. Well done, @jaeseung-bae!

grpc/src/main/java/com/linecorp/armeria/client/grpc/GrpcClientBuilder.java

grpc/src/main/java/com/linecorp/armeria/server/grpc/GrpcServiceBuilder.java

grpc/src/test/java/com/linecorp/armeria/client/grpc/GrpcClientBuilderTest.java

jrhee17

Left a minor comment on testing but looks good! Thanks @jaeseung-bae 🙇 👍 🙇

jrhee17 · 2024-05-07T07:22:45Z

grpc/src/test/java/com/linecorp/armeria/server/grpc/GrpcServiceBuilderTest.java

+        };
+
+        final TestServiceImplBase testService = new TestServiceImpl(
+                Executors.newSingleThreadScheduledExecutor());


Suggested change

Executors.newSingleThreadScheduledExecutor());

CommonPools.blockingTaskExecutor());

grpc/src/test/java/com/linecorp/armeria/server/grpc/GrpcServiceBuilderTest.java

minwoox

Basically, looks great! 👍 Left some suggestions. 😉

minwoox · 2024-05-07T07:13:09Z

grpc/src/main/java/com/linecorp/armeria/client/grpc/GrpcClientBuilder.java

@@ -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)) {


Could you move logic into enableUnsafeWrapResponseBuffers and useMethodMarshaller methods so that the exception is thrown early when the property is set?

Okay, I'll move logic to both enableUnsafeWrapResponseBuffers() and useMethodMarshaller() method.

Applied feedbacks, Thanks.

grpc/src/main/java/com/linecorp/armeria/internal/client/grpc/GrpcClientFactory.java

grpc/src/main/java/com/linecorp/armeria/internal/common/grpc/GrpcMessageMarshaller.java

grpc/src/main/java/com/linecorp/armeria/server/grpc/GrpcServiceBuilder.java

grpc/src/main/java/com/linecorp/armeria/internal/common/grpc/GrpcMessageMarshaller.java

minwoox

Excellent work, @jaeseung-bae! 👍 👍 👍
Thanks!

minwoox · 2024-05-16T07:28:52Z

@jaeseung-bae 👍 👍 👍

feat: add option to respect the marshaller specified in gRPC MethodDe…

b808a8b

…scriptor

jaeseung-bae marked this pull request as ready for review April 22, 2024 23:02

jaeseung-bae changed the title ~~feat: add option to respect the marshaller specified in gRPC MethodDe…~~ Add option to respect the marshaller specified in gRPC MethodDescriptor Apr 23, 2024

trustin reviewed Apr 23, 2024

View reviewed changes

jaeseung-bae and others added 3 commits April 24, 2024 08:23

Update grpc/src/main/java/com/linecorp/armeria/client/grpc/GrpcClient…

24fe248

…Builder.java Co-authored-by: Trustin Lee <t@motd.kr>

Update grpc/src/main/java/com/linecorp/armeria/client/grpc/GrpcClient…

e4fa595

…Options.java Co-authored-by: Trustin Lee <t@motd.kr>

test: add test for GrpcClientBuilder to set use-method-marshaller cor…

e11b59d

…rectly

ikhoon added new feature sprint Issues for OSS Sprint participants labels Apr 24, 2024

jaeseung-bae requested a review from trustin April 24, 2024 05:43

ikhoon reviewed Apr 24, 2024

View reviewed changes

test: update test for GrpcClientBuilder to set use-method-marshaller …

95d2ed1

…correctly

jaeseung-bae requested a review from ikhoon April 25, 2024 04:50

Merge branch 'main' into feat/use-method-marshaller

e361a5d

ikhoon reviewed Apr 25, 2024

View reviewed changes

jaeseung-bae and others added 3 commits April 26, 2024 07:14

Update grpc/src/test/java/com/linecorp/armeria/server/grpc/UnaryServe…

e848c98

…rCallTest.java Co-authored-by: Ikhun Um <ih.pert@gmail.com>

test: update test for Client and Server to set use-method-marshaller …

76ba74c

…correctly

Merge branch 'main' into feat/use-method-marshaller

e074d06

jaeseung-bae requested a review from ikhoon April 26, 2024 10:02

ikhoon reviewed Apr 26, 2024

View reviewed changes

ikhoon added this to the 1.29.0 milestone Apr 26, 2024

jaeseung-bae added 2 commits April 28, 2024 10:59

chore: move test helper class to the bottom code to focus on testcase…

6bd0be1

… well

chore: make call count variable as local and use different call count…

06c5268

… variable for both request and response. Add additional description for requestStreamCallCnt

ikhoon reviewed Apr 29, 2024

View reviewed changes

chore: add validate logic for 'unsafeWrapRequestBuffers' and 'useMeth…

5543dc1

…odMarshaller' mutually exclusive state

jaeseung-bae requested review from jrhee17 and minwoox as code owners April 29, 2024 14:44

trustin approved these changes Apr 30, 2024

View reviewed changes

jaeseung-bae requested a review from ikhoon April 30, 2024 11:23

chore: deserializeProto now checks useMethodMarshaller

7d25d00

jaeseung-bae force-pushed the feat/use-method-marshaller branch from dabd601 to 7d25d00 Compare April 30, 2024 11:29

chore: update test to check parse call count

73352e7

jrhee17 approved these changes May 7, 2024

View reviewed changes

minwoox reviewed May 7, 2024

View reviewed changes

ikhoon reviewed May 7, 2024

View reviewed changes

grpc/src/main/java/com/linecorp/armeria/internal/common/grpc/GrpcMessageMarshaller.java Outdated Show resolved Hide resolved

jaeseung-bae added 2 commits May 10, 2024 08:56

chore: apply feedback

00ea71b

chore: remove redundant call when useMethodMarshaller=true

33b67a4

minwoox approved these changes May 14, 2024

View reviewed changes

minwoox merged commit 8ab4284 into line:main May 16, 2024
15 checks passed

minwoox mentioned this pull request May 21, 2024

Provide a way to provide custom protobuf marshallers #5103

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add option to respect the marshaller specified in gRPC MethodDescriptor #5630

Add option to respect the marshaller specified in gRPC MethodDescriptor #5630

jaeseung-bae commented Apr 22, 2024 •

edited

github-actions bot commented Apr 22, 2024 •

edited

ikhoon Apr 24, 2024

jaeseung-bae Apr 24, 2024

jaeseung-bae Apr 25, 2024

ikhoon Apr 25, 2024

jaeseung-bae Apr 26, 2024 •

edited

ikhoon Apr 26, 2024

jaeseung-bae Apr 26, 2024

ikhoon left a comment

ikhoon Apr 26, 2024

jaeseung-bae Apr 28, 2024

ikhoon Apr 26, 2024

jaeseung-bae Apr 28, 2024

ikhoon Apr 26, 2024

jaeseung-bae Apr 28, 2024

ikhoon Apr 29, 2024

jaeseung-bae Apr 30, 2024

ikhoon Apr 29, 2024

jaeseung-bae Apr 30, 2024

trustin left a comment

jrhee17 left a comment

jrhee17 May 7, 2024

minwoox left a comment

minwoox May 7, 2024

jaeseung-bae May 9, 2024 •

edited

jaeseung-bae May 9, 2024

minwoox left a comment

minwoox commented May 16, 2024

	true);
	}

	private static Stream<Arguments> messageMarshallerArgsWithMethodMarshaller() {
	return grpcJsonMarshallerStream().map(GrpcMessageMarshallerTest::messageMarshallerWithMethodMarshaller)
	.map(Arguments::of);
	}

	@ParameterizedTest
	@MethodSource({"messageMarshallerArgs", "messageMarshallerArgsWithMethodMarshaller"})

		final AtomicInteger requestStreamCallCnt = new AtomicInteger(0);
		final AtomicInteger responseStreamCallCnt = new AtomicInteger(0);

	Executors.newSingleThreadScheduledExecutor());
	CommonPools.blockingTaskExecutor());

Add option to respect the marshaller specified in gRPC MethodDescriptor #5630

Add option to respect the marshaller specified in gRPC MethodDescriptor #5630

Conversation

jaeseung-bae commented Apr 22, 2024 • edited

github-actions bot commented Apr 22, 2024 • edited

🔍 Build Scan® (commit: 33b67a4)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaeseung-bae Apr 26, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ikhoon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trustin left a comment

Choose a reason for hiding this comment

jrhee17 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

minwoox left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaeseung-bae May 9, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

minwoox left a comment

Choose a reason for hiding this comment

minwoox commented May 16, 2024

jaeseung-bae commented Apr 22, 2024 •

edited

github-actions bot commented Apr 22, 2024 •

edited

🔍 Build Scan® (commit: `33b67a4`)

jaeseung-bae Apr 26, 2024 •

edited

jaeseung-bae May 9, 2024 •

edited