szetszwo commented on code in PR #1007:
URL: https://github.com/apache/ratis/pull/1007#discussion_r1447993487
##########
ratis-grpc/src/main/java/org/apache/ratis/grpc/GrpcConfigKeys.java:
##########
@@ -292,6 +292,16 @@ static void setLogMessageBatchDuration(RaftProperties
properties,
setTimeDuration(properties::setTimeDuration,
LOG_MESSAGE_BATCH_DURATION_KEY, logMessageBatchDuration);
}
+
+ String ZERO_COPY_ENABLED_KEY = PREFIX + ".zerocopy.enabled";
+ boolean ZERO_COPY_ENABLED_DEFAULT = false;
Review Comment:
It seems that there are no reasons to disable it. Let's don't add this conf?
##########
ratis-grpc/src/main/java/org/apache/ratis/grpc/server/GrpcClientProtocolService.java:
##########
@@ -278,15 +314,18 @@ private class UnorderedRequestStreamObserver extends
RequestStreamObserver {
}
@Override
- void processClientRequest(RaftClientRequest request) {
- final CompletableFuture<Void> f = processClientRequest(request, reply ->
{
+ void processClientRequest(ReferenceCountedObject<RaftClientRequest>
requestRef) {
+ final RaftClientRequest request = requestRef.retain();
+ final long callId = request.getCallId();
+
+ final CompletableFuture<Void> f = processClientRequest(requestRef, reply
-> {
if (!reply.isSuccess()) {
- LOG.info("Failed " + request + ", reply=" + reply);
+ LOG.info("Failed {}}, reply={}", request, reply);
Review Comment:
Typo: `Failed {}}`
##########
ratis-common/src/main/java/org/apache/ratis/protocol/RaftClientRequest.java:
##########
@@ -444,7 +444,15 @@ public long getTimeoutMs() {
@Override
public String toString() {
- return super.toString() + ", seq=" +
ProtoUtils.toString(slidingWindowEntry) + ", "
- + type + ", " + getMessage();
+ return toStringShort() + ", " + getMessage();
+ }
+
+ /**
+ * Safe version of toString that doesn't involve the message content.
+ *
+ * @return
+ */
Review Comment:
We should not have an empty `@return`. How about updating the javadoc to?
```java
/** @return a short string which does not include {@link #message}. */
```
##########
ratis-grpc/src/main/java/org/apache/ratis/grpc/server/GrpcClientProtocolService.java:
##########
@@ -220,31 +249,38 @@ boolean isClosed() {
return isClosed.get();
}
- CompletableFuture<Void> processClientRequest(RaftClientRequest request,
Consumer<RaftClientReply> replyHandler) {
- try {
- final String errMsg = LOG.isDebugEnabled() ? "processClientRequest for
" + request : "";
- return protocol.submitClientRequestAsync(request
- ).thenAcceptAsync(replyHandler, executor
- ).exceptionally(exception -> {
- // TODO: the exception may be from either raft or state machine.
- // Currently we skip all the following responses when getting an
- // exception from the state machine.
- responseError(exception, () -> errMsg);
- return null;
- });
- } catch (IOException e) {
- throw new CompletionException("Failed processClientRequest for " +
request + " in " + name, e);
- }
+ CompletableFuture<Void>
processClientRequest(ReferenceCountedObject<RaftClientRequest> requestRef,
+ Consumer<RaftClientReply> replyHandler) {
+ final String errMsg = LOG.isDebugEnabled() ? "processClientRequest for "
+ requestRef : "";
Review Comment:
We should get the request here.
--
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]
