Allow client compressed tests without probing (#4240)#4279
Allow client compressed tests without probing (#4240)#4279ericgribkoff merged 3 commits intogrpc:masterfrom
Conversation
|
Thank you for your pull request. Before we can look at your contribution, we need to ensure all contributors are covered by a Contributor License Agreement. After the following items are addressed, please respond with a new comment here, and the automated system will re-verify.
Regards, |
raboof
force-pushed
the
clientCompressionTestsWithoutProbing
branch
from
d898a4d to
c1ef426
Compare
|
Thank you for your pull request. Before we can look at your contribution, we need to ensure all contributors are covered by a Contributor License Agreement. After the following items are addressed, please respond with a new comment here, and the automated system will re-verify.
Regards, |
|
(signed the CLA) |
|
Could you elaborate on the motivation for skipping the initial RPC? Generally speaking, the test cases here come from the cross-language specification in https://github.com/grpc/grpc/blob/master/doc/interop-test-descriptions.md. The compression tests are already a little out-of-place for Java, since we can't inspect the compression level as is done in C++, so these are only really useful when run against the C++ server. From the comments on the new methods in this PR, this restriction also applies when the initial probing RPC is skipped. |
|
@ericgribkoff sure! there's a little more background in #4240. In short, servers that do not provide access to the message-level compression flag (like grpc-java, but I think this might go for other implementations?) cannot implement the server-side of the feature probe request. Because of that, running this test against such a server will fail at the feature probe step. When skipping the feature probe step, running this test against such a server will be able to exercise the compression and check for interoperability by checking the messages arrive in good order - even though it is indeed not explicitly checked that they are, in fact, compressed. |
ericgribkoff
added
the
kokoro:run
kokoro-team
removed
kokoro:run
|
Ah, got it, thanks - I missed the discussion in #4240. Just kicked off the tests for this PR. |
| CACHEABLE_UNARY("cacheable unary rpc sent using GET"), | ||
| LARGE_UNARY("single request and (large) response"), | ||
| CLIENT_COMPRESSED_UNARY("client compressed unary request"), | ||
| CLIENT_COMPRESSED_UNARY_NOPROBE("client compressed unary request (skip initial feature-probing request)"), |
There was a problem hiding this comment.
This line (and CLIENT_COMPRESSED_STREAMING_NOPROBE) exceeds 100 characters. Can you change them to the format:
CLIENT_COMPRESSED_UNARY_NOPROBE(
"client compressed unary request (skip initial feature-probing request)"),
| * Tests client per-message compression for unary calls. The Java API does not support inspecting | ||
| * a message's compression level, so this is primarily intended to run against a gRPC C++ server. | ||
| */ | ||
| public void clientCompressedUnaryNoProbe() throws Exception { |
There was a problem hiding this comment.
Didn't catch this earlier - checkstyle fails here due to the separation between clientCompressedUnary() and clientCompressedUnary(boolean). I'm ok with just dropping the no-arg methods (clientCompressedUnary() and clientCompressedUnaryNoProbe()) and directly invoking clientCompressedUnary(true or false) in TestServiceClient. Same for clientCompressedStreaming
ericgribkoff
added
the
kokoro:run
kokoro-team
removed
the
kokoro:run
4 participants
I chose the
_noprobesuffix instead of_novalidatesince it more accuratelydescribes the difference: the validations are the same, but the new variations
skip the feature-probing call.