[GitHub] [ratis] szetszwo commented on a change in pull request #563: RATIS-1438. Add request timeout to ratis Streaming

[GitBox <gi...@apache.org>]

szetszwo commented on a change in pull request #563:
URL: https://github.com/apache/ratis/pull/563#discussion_r769156710



##########
File path: ratis-client/src/main/java/org/apache/ratis/client/impl/OrderedStreamAsync.java
##########
@@ -147,4 +156,13 @@ private void sendRequestToNetwork(DataStreamWindowRequest request){
       return null;
     });
   }
+
+  private void scheduleWithTimeout(DataStreamWindowRequest request, TimeDuration sleepTime) {

Review comment:
       Let's use requestTimeout directly.
   ```
     private void scheduleWithTimeout(DataStreamWindowRequest request) {
       scheduler.onTimeout(requestTimeout, () -> {
         if (!request.getReplyFuture().isDone()) {
           request.getReplyFuture().completeExceptionally(
               new TimeoutIOException("Timeout " + requestTimeout + ": Failed to send " + request));
         }
       }, LOG, () -> "Failed to completeExceptionally for " + request);
     }
   ```
   

##########
File path: ratis-client/src/main/java/org/apache/ratis/client/impl/OrderedStreamAsync.java
##########
@@ -147,4 +156,13 @@ private void sendRequestToNetwork(DataStreamWindowRequest request){
       return null;
     });
   }
+
+  private void scheduleWithTimeout(DataStreamWindowRequest request, TimeDuration sleepTime) {
+    scheduler.onTimeout(sleepTime, () -> {
+      if (!request.getReplyFuture().isDone()) {
+        request.getReplyFuture().completeExceptionally(
+            new IOException("Send request timeout " + request));
+      }
+    }, LOG, () -> "Send request timeout " + request);

Review comment:
       The error message is for handling the exception thrown by the timeout code.  Let's change it.
   ```
       }, LOG, () -> "Failed to completeExceptionally for " + request);
   ```
   

##########
File path: ratis-client/src/main/java/org/apache/ratis/client/impl/OrderedStreamAsync.java
##########
@@ -147,4 +156,13 @@ private void sendRequestToNetwork(DataStreamWindowRequest request){
       return null;
     });
   }
+
+  private void scheduleWithTimeout(DataStreamWindowRequest request, TimeDuration sleepTime) {
+    scheduler.onTimeout(sleepTime, () -> {
+      if (!request.getReplyFuture().isDone()) {
+        request.getReplyFuture().completeExceptionally(
+            new IOException("Send request timeout " + request));

Review comment:
       - Let's use TimeoutIOException instead of IOException
   - Include the timeout value in the error message of TimeoutIOException.
   ```
               new TimeoutIOException("Timeout " + requestTimeout + ": Failed to send " + 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: issues-unsubscribe@ratis.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org