You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@hudi.apache.org by GitBox <gi...@apache.org> on 2022/08/29 20:20:21 UTC

[GitHub] [hudi] nsivabalan commented on a diff in pull request #6393: [HUDI-4619] Fix The retry mechanism of remotehoodietablefilesystemvie…

nsivabalan commented on code in PR #6393:
URL: https://github.com/apache/hudi/pull/6393#discussion_r957759152


##########
hudi-common/src/main/java/org/apache/hudi/common/table/view/RemoteHoodieTableFileSystemView.java:
##########
@@ -495,4 +496,14 @@ public Option<HoodieBaseFile> getLatestBaseFile(String partitionPath, String fil
       throw new HoodieRemoteException(e);
     }
   }
+
+  public static Response get(int timeoutMs, String url, RequestMethod method) throws IOException {

Review Comment:
   does this have to be public ?



##########
hudi-timeline-service/src/test/java/org/apache/hudi/timeline/service/functional/TestRemoteHoodieTableFileSystemView.java:
##########
@@ -64,4 +66,31 @@ protected SyncableFileSystemView getFileSystemView(HoodieTimeline timeline) {
     view = new RemoteHoodieTableFileSystemView("localhost", server.getServerPort(), metaClient);
     return view;
   }
+
+  @Test
+  public void testRemoteHoodieTableFileSystemViewWithRetry() {
+    // Service is available.
+    view.getLatestBaseFiles();
+    // Shut down the service.
+    server.close();
+    try {
+      // Immediately fails and throws a connection refused exception.
+      view.getLatestBaseFiles();
+    } catch (HoodieRemoteException e) {

Review Comment:
   don't we need to add Assert.fail() just after line 78?



##########
hudi-timeline-service/src/test/java/org/apache/hudi/timeline/service/functional/TestRemoteHoodieTableFileSystemView.java:
##########
@@ -64,4 +66,31 @@ protected SyncableFileSystemView getFileSystemView(HoodieTimeline timeline) {
     view = new RemoteHoodieTableFileSystemView("localhost", server.getServerPort(), metaClient);
     return view;
   }
+
+  @Test
+  public void testRemoteHoodieTableFileSystemViewWithRetry() {
+    // Service is available.
+    view.getLatestBaseFiles();
+    // Shut down the service.
+    server.close();
+    try {
+      // Immediately fails and throws a connection refused exception.
+      view.getLatestBaseFiles();
+    } catch (HoodieRemoteException e) {
+      assert e.getMessage().contains("Connection refused (Connection refused)");
+    }
+    // Enable API request retry for remote file system view.
+    view =  new RemoteHoodieTableFileSystemView(metaClient, FileSystemViewStorageConfig
+            .newBuilder()
+            .withRemoteServerHost("localhost")
+            .withRemoteServerPort(server.getServerPort())
+            .withRemoteTimelineClientRetry(true)
+            .withRemoteTimelineClientMaxRetryNumbers(4)
+            .build());
+    try {
+      view.getLatestBaseFiles();

Review Comment:
   don't we need to add Assert.fail() just after line 91?



##########
hudi-timeline-service/src/test/java/org/apache/hudi/timeline/service/functional/TestRemoteHoodieTableFileSystemView.java:
##########
@@ -64,4 +66,31 @@ protected SyncableFileSystemView getFileSystemView(HoodieTimeline timeline) {
     view = new RemoteHoodieTableFileSystemView("localhost", server.getServerPort(), metaClient);
     return view;
   }
+
+  @Test
+  public void testRemoteHoodieTableFileSystemViewWithRetry() {
+    // Service is available.
+    view.getLatestBaseFiles();
+    // Shut down the service.
+    server.close();
+    try {
+      // Immediately fails and throws a connection refused exception.
+      view.getLatestBaseFiles();
+    } catch (HoodieRemoteException e) {
+      assert e.getMessage().contains("Connection refused (Connection refused)");
+    }
+    // Enable API request retry for remote file system view.
+    view =  new RemoteHoodieTableFileSystemView(metaClient, FileSystemViewStorageConfig
+            .newBuilder()
+            .withRemoteServerHost("localhost")
+            .withRemoteServerPort(server.getServerPort())
+            .withRemoteTimelineClientRetry(true)
+            .withRemoteTimelineClientMaxRetryNumbers(4)
+            .build());
+    try {
+      view.getLatestBaseFiles();

Review Comment:
   is it no possible to test that retry succeed after 2 or 3 tries? 



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