Re: [PR] RATIS-2404. Validate message size before sending async requests. [ratis]
slfan1989 commented on PR #1345: URL: https://github.com/apache/ratis/pull/1345#issuecomment-3881481860 > +1 the change looks good. @szetszwo Thank you so much for reviewing the code! -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] RATIS-2404. Validate message size before sending async requests. [ratis]
szetszwo merged PR #1345: URL: https://github.com/apache/ratis/pull/1345 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] RATIS-2404. Validate message size before sending async requests. [ratis]
slfan1989 commented on code in PR #1345:
URL: https://github.com/apache/ratis/pull/1345#discussion_r2790434089
##
ratis-grpc/src/main/java/org/apache/ratis/grpc/client/GrpcClientProtocolClient.java:
##
@@ -236,6 +237,10 @@ public RaftPeer getTarget() {
return target;
}
+ private RaftClientRequestProto toRaftClientRequestProto(RaftClientRequest
request) throws IOException {
+return ClientProtoUtils.toRaftClientRequestProto(request);
+ }
Review Comment:
Thanks for your suggestions—I’ve updated and improved the code.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] RATIS-2404. Validate message size before sending async requests. [ratis]
szetszwo commented on code in PR #1345:
URL: https://github.com/apache/ratis/pull/1345#discussion_r2790392624
##
ratis-grpc/src/main/java/org/apache/ratis/grpc/client/GrpcClientProtocolClient.java:
##
@@ -236,6 +237,10 @@ public RaftPeer getTarget() {
return target;
}
+ private RaftClientRequestProto toRaftClientRequestProto(RaftClientRequest
request) throws IOException {
+return ClientProtoUtils.toRaftClientRequestProto(request);
+ }
Review Comment:
@slfan1989 , thanks for update!
Please remove this method and use
ClientProtoUtils.toRaftClientRequestProto(..) directly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] RATIS-2404. Validate message size before sending async requests. [ratis]
slfan1989 commented on code in PR #1345:
URL: https://github.com/apache/ratis/pull/1345#discussion_r2790346484
##
ratis-grpc/src/main/java/org/apache/ratis/grpc/client/GrpcClientProtocolClient.java:
##
@@ -337,8 +348,15 @@ CompletableFuture
onNext(RaftClientRequest request) {
if (f == null) {
return JavaUtils.completeExceptionally(new
AlreadyClosedException(getName() + " is closed."));
}
+ final RaftClientRequestProto proto;
+ try {
+proto = toRaftClientRequestProto(request);
Review Comment:
Thanks for the review patch — I’ve already updated and improved the code
accordingly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] RATIS-2404. Validate message size before sending async requests. [ratis]
slfan1989 commented on code in PR #1345:
URL: https://github.com/apache/ratis/pull/1345#discussion_r2790337170
##
ratis-grpc/src/main/java/org/apache/ratis/grpc/client/GrpcClientProtocolClient.java:
##
@@ -121,7 +123,7 @@ public class GrpcClientProtocolClient implements Closeable {
}
private ManagedChannel buildChannel(String address, SslContext sslContext,
- SizeInBytes flowControlWindow, SizeInBytes maxMessageSize) {
+ SizeInBytes flowControlWindow, SizeInBytes maxMessageSizeConfig) {
Review Comment:
Agreed — I’ll remove the parameter since it’s now a field.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] RATIS-2404. Validate message size before sending async requests. [ratis]
slfan1989 commented on code in PR #1345:
URL: https://github.com/apache/ratis/pull/1345#discussion_r2790335054
##
ratis-grpc/src/main/java/org/apache/ratis/grpc/client/GrpcClientProtocolClient.java:
##
@@ -82,6 +82,7 @@ public class GrpcClientProtocolClient implements Closeable {
private final ManagedChannel clientChannel;
private final ManagedChannel adminChannel;
+ private final int maxMessageSize;
Review Comment:
Agreed. Let’s use SizeInBytes. The string representation should be fine when
we print it out.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] RATIS-2404. Validate message size before sending async requests. [ratis]
szetszwo commented on code in PR #1345:
URL: https://github.com/apache/ratis/pull/1345#discussion_r2789342000
##
ratis-grpc/src/main/java/org/apache/ratis/grpc/client/GrpcClientProtocolClient.java:
##
@@ -82,6 +82,7 @@ public class GrpcClientProtocolClient implements Closeable {
private final ManagedChannel clientChannel;
private final ManagedChannel adminChannel;
+ private final int maxMessageSize;
Review Comment:
Let's use `SizeInBytes`. The String will look when printing it out.
##
ratis-grpc/src/main/java/org/apache/ratis/grpc/client/GrpcClientProtocolClient.java:
##
@@ -121,7 +123,7 @@ public class GrpcClientProtocolClient implements Closeable {
}
private ManagedChannel buildChannel(String address, SslContext sslContext,
- SizeInBytes flowControlWindow, SizeInBytes maxMessageSize) {
+ SizeInBytes flowControlWindow, SizeInBytes maxMessageSizeConfig) {
Review Comment:
Since it becomes a field, let's remove the parameter.
##
ratis-grpc/src/main/java/org/apache/ratis/grpc/client/GrpcClientProtocolClient.java:
##
@@ -337,8 +348,15 @@ CompletableFuture
onNext(RaftClientRequest request) {
if (f == null) {
return JavaUtils.completeExceptionally(new
AlreadyClosedException(getName() + " is closed."));
}
+ final RaftClientRequestProto proto;
+ try {
+proto = toRaftClientRequestProto(request);
Review Comment:
This is a good idea to validate to request first. Let's check it in the
very beginning of this method.
##
ratis-grpc/src/main/java/org/apache/ratis/grpc/client/GrpcClientProtocolClient.java:
##
@@ -236,6 +238,15 @@ public RaftPeer getTarget() {
return target;
}
+ private RaftClientRequestProto toRaftClientRequestProto(RaftClientRequest
request) throws IOException {
+final RaftClientRequestProto proto =
ClientProtoUtils.toRaftClientRequestProto(request);
+if (proto.getSerializedSize() > maxMessageSize) {
+ throw new IOException(getName() + ": Message size:" +
proto.getSerializedSize()
+ + " exceeds maximum:" + maxMessageSize);
Review Comment:
- Use IllegalArgumentException
- Use "request serialized size" instead of "Message size"
- Include the request.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] RATIS-2404. Validate message size before sending async requests. [ratis]
slfan1989 commented on PR #1345: URL: https://github.com/apache/ratis/pull/1345#issuecomment-3878703320 @szetszwo Could you please review this PR? Thank you very much! -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
