MCS connection scaling interop tests for Java by kannanjgithub · Pull Request #12651 · grpc/grpc-java

kannanjgithub · 2026-02-10T12:43:18Z

No description provided.

ejona86 · 2026-02-11T19:59:12Z

interop-testing/src/main/java/io/grpc/testing/integration/TestServiceClient.java

      }
-
+
+      case MCS_CS: {


If there's a new test, then that should be defined in https://github.com/grpc/grpc/blob/master/doc/interop-test-descriptions.md

Will do in the PR on the core repo.

ejona86 · 2026-02-11T19:59:52Z

interop-testing/src/main/java/io/grpc/testing/integration/TestServiceClient.java

+          try {
+            @SuppressWarnings("unchecked")
+            Map<String, ?> serviceConfigMap = (Map<String, ?>) JsonParser.parse(
+                "{\"connection_scaling\":{\"max_connections_per_subchannel\": 2}}");


We don't support connection scaling yet. So this test has never succeeded?

Yes, added a comment on the failing assertion now.

ejona86 · 2026-02-11T20:01:24Z

interop-testing/src/main/java/io/grpc/testing/integration/TestServiceClient.java

          channelBuilder.overrideAuthority(serverHostOverride);
        }
-        if (serviceConfig != null) {
+        if (testCase.equals(MCS_CS.toString())) {


Instead of hacking up this method, can we create the channel within the scope of the test, like GOOGLE_DEFAULT_CREDENTIALS?

ejona86 · 2026-02-11T20:05:40Z

interop-testing/src/main/java/io/grpc/testing/integration/TestServiceClient.java

          .setOrcaOobReport(answer2)
          .addResponseParameters(ResponseParameters.newBuilder().setSize(1).build()).build());
-      assertThat(queue.take()).isInstanceOf(StreamingOutputCallResponse.class);
+      assertThat(streamingOutputCallResponseObserver.isCompleted).isTrue();


I don't see how this is an okay transformation. Previously we were asserting there was a message. Why would we stop doing that?

Note that using isCompleted is weaker than what was there before.

Because it is racy; take() was waiting until events occurred

Because it doesn't verify that extra messages weren't received

It should always fail, because streamObserver.onCompleted(); caused the server to close the stream.

Some statements got messed up unintentionally regarding asserting message and stream completion.

Restored the blocking queue.take() approach for determining stream completion to avoid race.

ejona86 · 2026-02-11T20:06:41Z

interop-testing/src/main/java/io/grpc/testing/integration/TestServiceClient.java

+
+    class StreamingOutputCallResponseObserver implements
+        StreamObserver<StreamingOutputCallResponse> {
+      private final BlockingQueue<Object> queue = new LinkedBlockingQueue<>();


The only reason this held Object was to store lastItem in it. I do think we want to preserve that approach, but if we didn't, we should change this to BlockingQueue<StreamingOutputCallResponse>

Preserved the existing approach to identify rpc completion.

ejona86 · 2026-02-11T20:13:49Z

interop-testing/src/main/java/io/grpc/testing/integration/TestServiceImpl.java

          return;
        }
+        if (new String(request.getPayload().getBody().toByteArray(), UTF_8)
+            .equals(MCS_CS.description())) {


Please no. There's two regular approaches we use for tweaking the server's behavior:

A field in the request proto (e.g., fill_username)

Header metadata. I don't think we actually use this approach in the vanilla interop today, but we do do it for psm interop

(fill_username and the like are because of mistakes in the past where grpc-java verified the result message exactly. That means you can't add new fields to have them be ignored. We did want to verify the result was what we expected, but it would have probably been better to just verify the payload. But the behavior hasn't been changed, so we still need these knobs that enable result fields.)

Added new fields in request and response for setting the client socket address.

ejona86 · 2026-02-11T20:15:11Z

interop-testing/src/main/java/io/grpc/testing/integration/TestServiceImpl.java

+              StreamingOutputCallResponse.newBuilder();
+          responseBuilder.setPayload(
+              Payload.newBuilder()
+                  .setBody(payload));


We should store the result in a new field of the proto or in metadata.

The server shouldn't know what specific test is being run. It should just be told what to do and do it. And we want to allow mixing-and-matching the things the server can be told to do (e.g., if you want fill_username and fill_client_address (made up example) at the same time, you can)

ejona86 · 2026-02-11T20:20:18Z

interop-testing/src/main/java/io/grpc/testing/integration/TestServiceServer.java

          usage = true;
          break;
        }
+      } else if ("use_mcs".equals(key)) {


How about we let this be --mcs=2 (or not abbreviated, since mcs isn't clear without context), where the caller can specify what to limit the concurrency limit to.

I'd considered specifying the count as a value, but any value other than 2 would cause the test to fail, that's why I set the value in the server code.

ejona86 · 2026-02-11T20:26:08Z

interop-testing/src/main/java/io/grpc/testing/integration/TestServiceServer.java

          usage = true;
          break;
        }
+      } else if ("use_mcs".equals(key)) {


Update usage output for the new argument

ejona86 · 2026-02-11T20:29:37Z

interop-testing/src/main/java/io/grpc/testing/integration/TestServiceServer.java

        }
+      } else if ("use_mcs".equals(key)) {
+        useMcs = Boolean.parseBoolean(value);
+        addressType = Util.AddressType.IPV4; // To use NettyServerBuilder


That's unfortunate. We really do want to support IPv6; IPv6-only is increasingly common. Also, it'd be good to stop hard-coding Netty for IPv4/IPv6. When this was done we only had the Netty server, but now we have the OkHttp server as well.

But it also looks pretty hard to fix in a "cleaner" way.

Added a TODO to fix Netty server builder usage to work with either IPv4/6.

… test.

kannanjgithub added 13 commits January 19, 2026 06:40

Save changes.

fb52bbf

Save changes.

f518d5e

Save changes.

3874631

Save changes.

0bad82f

Fix enum test.

51b38e2

Revert temp changes.

4da06a4

Revert temp changes.

df70a27

Try with server streaming alone.

bee005f

Interceptor logic.

efce818

Closing the stream after the rpc is done.

2ac2d47

Style fixes.

4214889

Fix test name.

e099d1d

Fix style warnings.

3d75bf8

kannanjgithub requested a review from ejona86 February 10, 2026 16:10

Fix build.

84d9528

ejona86 reviewed Feb 11, 2026

View reviewed changes

kannanjgithub added 2 commits February 12, 2026 12:44

Review comments - Build channel separately for MCS connection scaling…

3136bca

… test.

Address Review comments.

c3fc7c3

Conversation

kannanjgithub commented Feb 10, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants