You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ratis.apache.org by GitBox <gi...@apache.org> on 2021/12/14 12:43:02 UTC

[GitHub] [ratis] guohao-rosicky opened a new pull request #563: RATIS-1438. Add request timeout to ratis Streaming

guohao-rosicky opened a new pull request #563:
URL: https://github.com/apache/ratis/pull/563


   ## What changes were proposed in this pull request?
   
   Add request timeout to ratis Streaming
   
   ## What is the link to the Apache JIRA
   
   https://issues.apache.org/jira/browse/RATIS-1438


-- 
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

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

Posted by 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

[GitHub] [ratis] guohao-rosicky commented on pull request #563: RATIS-1438. Add request timeout to ratis Streaming

Posted by GitBox <gi...@apache.org>.
guohao-rosicky commented on pull request #563:
URL: https://github.com/apache/ratis/pull/563#issuecomment-993631125


   @szetszwo Could you help review this.


-- 
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

[GitHub] [ratis] szetszwo merged pull request #563: RATIS-1438. Add request timeout to ratis Streaming

Posted by GitBox <gi...@apache.org>.
szetszwo merged pull request #563:
URL: https://github.com/apache/ratis/pull/563


   


-- 
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

[GitHub] [ratis] captainzmc commented on pull request #563: RATIS-1438. Add request timeout to ratis Streaming

Posted by GitBox <gi...@apache.org>.
captainzmc commented on pull request #563:
URL: https://github.com/apache/ratis/pull/563#issuecomment-993525839


   Thanks @guohao-rosicky for your contribution.
    I did encounter the problem of Ozone Streaming client hung in the test. Although we have recently fixed many abnormal unhandled reply issues, but the client hung still will happen in long running test. 
   This PR can completely solve this problem.


-- 
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

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

Posted by GitBox <gi...@apache.org>.
guohao-rosicky commented on a change in pull request #563:
URL: https://github.com/apache/ratis/pull/563#discussion_r769191959



##########
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:
       Has been modified @szetszwo , please take a look




-- 
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