RATIS-2404. Validate message size before sending async requests. #1345
Conversation
|
@szetszwo Could you please review this PR? Thank you very much! |
szetszwo
left a comment
szetszwo
left a comment
There was a problem hiding this comment.
@slfan1989 , thanks a lot for working on this!
Please see the comments inlined and also https://issues.apache.org/jira/secure/attachment/13080732/1345_review.patch
| private final ManagedChannel clientChannel; | ||
| private final ManagedChannel adminChannel; | ||
|
|
||
| private final int maxMessageSize; |
There was a problem hiding this comment.
Let's use SizeInBytes. The String will look when printing it out.
There was a problem hiding this comment.
Agreed. Let’s use SizeInBytes. The string representation should be fine when we print it out.
|
|
||
| private ManagedChannel buildChannel(String address, SslContext sslContext, | ||
| SizeInBytes flowControlWindow, SizeInBytes maxMessageSize) { | ||
| SizeInBytes flowControlWindow, SizeInBytes maxMessageSizeConfig) { |
There was a problem hiding this comment.
Since it becomes a field, let's remove the parameter.
There was a problem hiding this comment.
Agreed — I’ll remove the parameter since it’s now a field.
| } | ||
| final RaftClientRequestProto proto; | ||
| try { | ||
| proto = toRaftClientRequestProto(request); |
There was a problem hiding this comment.
This is a good idea to validate to request first. Let's check it in the very beginning of this method.
There was a problem hiding this comment.
Thanks for the review patch — I’ve already updated and improved the code accordingly.
| throw new IOException(getName() + ": Message size:" + proto.getSerializedSize() | ||
| + " exceeds maximum:" + maxMessageSize); |
There was a problem hiding this comment.
- Use IllegalArgumentException
- Use "request serialized size" instead of "Message size"
- Include the request.
| private RaftClientRequestProto toRaftClientRequestProto(RaftClientRequest request) throws IOException { | ||
| return ClientProtoUtils.toRaftClientRequestProto(request); | ||
| } |
There was a problem hiding this comment.
@slfan1989 , thanks for update!
Please remove this method and use ClientProtoUtils.toRaftClientRequestProto(..) directly.
There was a problem hiding this comment.
Thanks for your suggestions—I’ve updated and improved the code.
szetszwo
left a comment
szetszwo
left a comment
There was a problem hiding this comment.
+1 the change looks good.
@szetszwo Thank you so much for reviewing the code! |
2 participants
What changes were proposed in this pull request?
Add client-side message size validation for async requests to fail fast before sending oversized messages over the network.
Currently, async requests are only validated on the server side, causing unnecessary network overhead. This change adds early validation in GrpcClientProtocolClient.asyncSend() to match the existing behavior
in the sync path (GrpcClientRpc).
What is the link to the Apache JIRA
RATIS-2404. Validate message size before sending async requests.
How was this patch tested?
Add Junit Test.