[GitHub] [hudi] yihua commented on a diff in pull request #5884: [HUDI-3669] Add a remote request retry mechanism for 'Remotehoodietablefiles…

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

yihua commented on code in PR #5884:
URL: https://github.com/apache/hudi/pull/5884#discussion_r955364712


##########
hudi-common/src/main/java/org/apache/hudi/common/table/view/RemoteHoodieTableFileSystemView.java:
##########
@@ -165,17 +179,9 @@ private <T> T executeRequest(String requestPath, Map<String, String> queryParame
 
     String url = builder.toString();
     LOG.info("Sending request : (" + url + ")");
-    Response response;
-    int timeout = this.timeoutSecs * 1000; // msec
-    switch (method) {
-      case GET:
-        response = Request.Get(url).connectTimeout(timeout).socketTimeout(timeout).execute();
-        break;
-      case POST:
-      default:
-        response = Request.Post(url).connectTimeout(timeout).socketTimeout(timeout).execute();
-        break;
-    }
+    // Reset url and method, to avoid repeatedly instantiating objects.
+    urlCheckedFunc.setUrlAndMethod(url, method);
+    Response response =  retryHelper != null ? retryHelper.tryWith(urlCheckedFunc).start() : urlCheckedFunc.get();

Review Comment:
   @LinMingQiang @danny0405 Every request goes through this flow and the `urlCheckedFunc` should not be shared across requests.  The logic here is incorrect.  This can cause serious correctness problems under concurrency.
   
   We need to revert this logic.  Also suggest that we should guard against such changes in the hot path with a flag.
   
   cc @nsivabalan @rmahindra123 



-- 
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: commits-unsubscribe@hudi.apache.org

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