MCS connection scaling interop tests for Java #12651
Conversation
| } | ||
|
|
||
|
|
||
| case MCS_CS: { |
There was a problem hiding this comment.
If there's a new test, then that should be defined in https://github.com/grpc/grpc/blob/master/doc/interop-test-descriptions.md
| try { | ||
| @SuppressWarnings("unchecked") | ||
| Map<String, ?> serviceConfigMap = (Map<String, ?>) JsonParser.parse( | ||
| "{\"connection_scaling\":{\"max_connections_per_subchannel\": 2}}"); |
There was a problem hiding this comment.
We don't support connection scaling yet. So this test has never succeeded?
| channelBuilder.overrideAuthority(serverHostOverride); | ||
| } | ||
| if (serviceConfig != null) { | ||
| if (testCase.equals(MCS_CS.toString())) { |
There was a problem hiding this comment.
Instead of hacking up this method, can we create the channel within the scope of the test, like GOOGLE_DEFAULT_CREDENTIALS?
| .setOrcaOobReport(answer2) | ||
| .addResponseParameters(ResponseParameters.newBuilder().setSize(1).build()).build()); | ||
| assertThat(queue.take()).isInstanceOf(StreamingOutputCallResponse.class); | ||
| assertThat(streamingOutputCallResponseObserver.isCompleted).isTrue(); |
There was a problem hiding this comment.
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.
|
|
||
| class StreamingOutputCallResponseObserver implements | ||
| StreamObserver<StreamingOutputCallResponse> { | ||
| private final BlockingQueue<Object> queue = new LinkedBlockingQueue<>(); |
There was a problem hiding this comment.
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>
| return; | ||
| } | ||
| if (new String(request.getPayload().getBody().toByteArray(), UTF_8) | ||
| .equals(MCS_CS.description())) { |
There was a problem hiding this comment.
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.)
| StreamingOutputCallResponse.newBuilder(); | ||
| responseBuilder.setPayload( | ||
| Payload.newBuilder() | ||
| .setBody(payload)); |
There was a problem hiding this comment.
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)
| usage = true; | ||
| break; | ||
| } | ||
| } else if ("use_mcs".equals(key)) { |
There was a problem hiding this comment.
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.
| usage = true; | ||
| break; | ||
| } | ||
| } else if ("use_mcs".equals(key)) { |
There was a problem hiding this comment.
Update usage output for the new argument
| } | ||
| } else if ("use_mcs".equals(key)) { | ||
| useMcs = Boolean.parseBoolean(value); | ||
| addressType = Util.AddressType.IPV4; // To use NettyServerBuilder |
There was a problem hiding this comment.
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.
2 participants
No description provided.