[#9280] improvement(catalogs-fileset): Refactor FileSystem retrieval to use `future` and solve hang problem by yuqi1129 · Pull Request #9282 · apache/gravitino

yuqi1129 · 2025-11-27T08:30:16Z

What changes were proposed in this pull request?

Replaced Awaitility-based polling with CompletableFuture for FileSystem retrieval using a dedicated ThreadPoolExecutor. This improves performance and simplifies timeout handling by leveraging async execution and modern concurrency utilities. Added cancellation handling and clearer exception propagation for robustness.

Why are the changes needed?

provider.getFileSystem(path, config) may hang and will never return a value, which will cause the Awaitility mechanism does not work as expected.

Fix: #9280

Does this PR introduce any user-facing change?

N/A

How was this patch tested?

Test locally and existing tests.

Replaced Awaitility-based polling with CompletableFuture for FileSystem retrieval using a dedicated ThreadPoolExecutor. This improves performance and simplifies timeout handling by leveraging async execution and modern concurrency utilities. Added cancellation handling and clearer exception propagation for robustness.

yuqi1129 · 2025-11-27T12:51:29Z

Gravitino still hangs on, although this has nothing to do with file system initialization, the following is the stack:

	at java.lang.Thread.sleep(java.base@17.0.8/Native Method)
	at com.amazonaws.http.AmazonHttpClient$RequestExecutor.doPauseBeforeRetry(AmazonHttpClient.java:1867)
	at com.amazonaws.http.AmazonHttpClient$RequestExecutor.pauseBeforeRetry(AmazonHttpClient.java:1841)
	at com.amazonaws.http.AmazonHttpClient$RequestExecutor.executeOneRequest(AmazonHttpClient.java:1282)
	at com.amazonaws.http.AmazonHttpClient$RequestExecutor.executeHelper(AmazonHttpClient.java:1145)
	at com.amazonaws.http.AmazonHttpClient$RequestExecutor.doExecute(AmazonHttpClient.java:802)
	at com.amazonaws.http.AmazonHttpClient$RequestExecutor.executeWithTimer(AmazonHttpClient.java:770)
	at com.amazonaws.http.AmazonHttpClient$RequestExecutor.execute(AmazonHttpClient.java:744)
	at com.amazonaws.http.AmazonHttpClient$RequestExecutor.access$500(AmazonHttpClient.java:704)
	at com.amazonaws.http.AmazonHttpClient$RequestExecutionBuilderImpl.execute(AmazonHttpClient.java:686)
	at com.amazonaws.http.AmazonHttpClient.execute(AmazonHttpClient.java:550)
	at com.amazonaws.http.AmazonHttpClient.execute(AmazonHttpClient.java:530)
	at com.amazonaws.services.s3.AmazonS3Client.invoke(AmazonS3Client.java:5227)
	at com.amazonaws.services.s3.AmazonS3Client.invoke(AmazonS3Client.java:5173)
	at com.amazonaws.services.s3.AmazonS3Client.getObjectMetadata(AmazonS3Client.java:1360)
	at org.apache.hadoop.fs.s3a.S3AFileSystem.lambda$getObjectMetadata$6(S3AFileSystem.java:2066)
	at org.apache.hadoop.fs.s3a.S3AFileSystem$$Lambda$790/0x000000f8017b9a18.apply(Unknown Source)
	at org.apache.hadoop.fs.s3a.Invoker.retryUntranslated(Invoker.java:412)
	at org.apache.hadoop.fs.s3a.Invoker.retryUntranslated(Invoker.java:375)
	at org.apache.hadoop.fs.s3a.S3AFileSystem.getObjectMetadata(S3AFileSystem.java:2056)
	at org.apache.hadoop.fs.s3a.S3AFileSystem.getObjectMetadata(S3AFileSystem.java:2032)
	at org.apache.hadoop.fs.s3a.S3AFileSystem.s3GetFileStatus(S3AFileSystem.java:3273)
	at org.apache.hadoop.fs.s3a.S3AFileSystem.innerGetFileStatus(S3AFileSystem.java:3185)
	at org.apache.hadoop.fs.s3a.S3AFileSystem.getFileStatus(S3AFileSystem.java:3053)
	at org.apache.hadoop.fs.FileSystem.exists(FileSystem.java:1760)
	at org.apache.hadoop.fs.s3a.S3AFileSystem.exists(S3AFileSystem.java:4263)
	at org.apache.gravitino.catalog.fileset.FilesetCatalogOperations.createMultipleLocationFileset(FilesetCatalogOperations.java:499)

Let me solve it by the way.

Copilot

Pull request overview

This PR refactors FileSystem retrieval to use CompletableFuture instead of Awaitility-based polling to address a hang issue where provider.getFileSystem(path, config) could hang indefinitely. The changes include adding a dedicated ThreadPoolExecutor for async FileSystem retrieval with proper timeout and cancellation handling, plus adding default timeout configurations to various FileSystem providers (HDFS, GCS, Azure, S3, OSS) to speed up test failures.

Replaces Awaitility polling with Future.get() with timeout for more robust timeout handling
Introduces a static ThreadPoolExecutor for async FileSystem retrieval operations
Adds default connection timeout and retry configurations across all FileSystem providers

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 8 comments.

Show a summary per file

File	Description
FilesetCatalogOperations.java	Replaces Awaitility with Future-based timeout mechanism using a new ThreadPoolExecutor
HDFSFileSystemProvider.java	Adds default HDFS connection timeout and ping configurations
GCSFileSystemProvider.java	Adds default GCS HTTP connect timeout and retry limit configurations
AzureFileSystemProvider.java	Adds default Azure retry limit configuration
S3FileSystemProvider.java	Adds default S3 retry limits and connection timeout configurations
OSSFileSystemProvider.java	Adds default OSS connection timeout and retry limit configurations

...log-fileset/src/main/java/org/apache/gravitino/catalog/fileset/FilesetCatalogOperations.java

...doop-common/src/main/java/org/apache/gravitino/catalog/hadoop/fs/HDFSFileSystemProvider.java

...log-fileset/src/main/java/org/apache/gravitino/catalog/fileset/FilesetCatalogOperations.java

Reduced thread pool keep-alive time from 50ms to 5s for better resource management. Added proper shutdown for the executor and improved logging for filesystem retrieval timeouts to enhance debugging and reliability.

Copilot

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.

bundles/aws/src/main/java/org/apache/gravitino/s3/fs/S3FileSystemProvider.java

bundles/aliyun/src/main/java/org/apache/gravitino/oss/fs/OSSFileSystemProvider.java

...log-fileset/src/main/java/org/apache/gravitino/catalog/fileset/FilesetCatalogOperations.java

...doop-common/src/main/java/org/apache/gravitino/catalog/hadoop/fs/HDFSFileSystemProvider.java

bundles/gcp/src/main/java/org/apache/gravitino/gcs/fs/GCSFileSystemProvider.java

bundles/azure/src/main/java/org/apache/gravitino/abs/fs/AzureFileSystemProvider.java

…ations

bundles/aliyun/src/main/java/org/apache/gravitino/oss/fs/OSSFileSystemProvider.java

jerryshao · 2025-12-04T07:17:29Z

...log-fileset/src/main/java/org/apache/gravitino/catalog/fileset/FilesetCatalogOperations.java


+  private final ThreadPoolExecutor fileSystemExecutor =
+      new ThreadPoolExecutor(
+          Math.max(2, Runtime.getRuntime().availableProcessors() * 2),


This will create too many threads for server if they're more than 32 cores.

I limit the max thread number to 24

...fileset/src/test/java/org/apache/gravitino/catalog/fileset/TestFilesetCatalogOperations.java

Copilot

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 16 comments.

...log-fileset/src/main/java/org/apache/gravitino/catalog/fileset/FilesetCatalogOperations.java

...fileset/src/test/java/org/apache/gravitino/catalog/fileset/TestFilesetCatalogOperations.java

bundles/azure/src/main/java/org/apache/gravitino/abs/fs/AzureFileSystemProvider.java

...log-fileset/src/main/java/org/apache/gravitino/catalog/fileset/FilesetCatalogOperations.java

...doop-common/src/main/java/org/apache/gravitino/catalog/hadoop/fs/HDFSFileSystemProvider.java

bundles/aliyun/src/main/java/org/apache/gravitino/oss/fs/OSSFileSystemProvider.java

...log-fileset/src/main/java/org/apache/gravitino/catalog/fileset/FilesetCatalogOperations.java

Copilot

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.

Copilot · 2025-12-05T03:50:38Z

...fileset/src/test/java/org/apache/gravitino/catalog/fileset/TestFilesetCatalogOperations.java

+      // Test the following method should finish with 10s
+      long now = System.currentTimeMillis();
+      try {
+        filesetCatalogOperations.getFileSystem(new Path("file:///tmp"), ImmutableMap.of());
+      } catch (IOException e) {
+        long timeTake = System.currentTimeMillis() - now;
+        Assertions.assertTrue(timeTake <= 10000);
+      }


The test doesn't verify that the exception is actually thrown - it silently catches IOException and only checks the timing. If no exception is thrown, the test will pass incorrectly. Consider using Assertions.assertThrows() to ensure an IOException is thrown, then verify the timing within that context.

Suggested change

// Test the following method should finish with 10s

long now = System.currentTimeMillis();

try {

filesetCatalogOperations.getFileSystem(new Path("file:///tmp"), ImmutableMap.of());

} catch (IOException e) {

long timeTake = System.currentTimeMillis() - now;

Assertions.assertTrue(timeTake <= 10000);

}

// Test the following method should finish with 10s and throw IOException

Assertions.assertThrows(

IOException.class,

() -> {

long now = System.currentTimeMillis();

try {

filesetCatalogOperations.getFileSystem(new Path("file:///tmp"), ImmutableMap.of());

} finally {

long timeTake = System.currentTimeMillis() - now;

Assertions.assertTrue(timeTake <= 10000, "Timeout should occur within 10 seconds");

}

});

bundles/gcp/src/main/java/org/apache/gravitino/gcs/fs/GCSFileSystemProvider.java

Copilot · 2025-12-05T03:50:38Z

...log-fileset/src/main/java/org/apache/gravitino/catalog/fileset/FilesetCatalogOperations.java

+  private final ThreadPoolExecutor fileSystemExecutor =
+      new ThreadPoolExecutor(
+          Math.max(2, Math.min(Runtime.getRuntime().availableProcessors() * 2, 16)),
+          Math.max(2, Math.min(Runtime.getRuntime().availableProcessors() * 2, 32)),
+          5L,
+          TimeUnit.SECONDS,
+          new ArrayBlockingQueue<>(1000),
+          new ThreadFactoryBuilder()
+              .setDaemon(true)
+              .setNameFormat("fileset-filesystem-getter-pool-%d")
+              .build(),
+          new ThreadPoolExecutor.AbortPolicy()) {
+        {
+          allowCoreThreadTimeOut(true);
+        }
+      };


The ThreadPoolExecutor is initialized as a final instance field, but the close() method calls shutdownNow() without checking if it has already been shut down. If close() is called multiple times, this could throw an exception or cause issues. Consider adding a guard to check the executor's state before shutting it down, or use a safer shutdown pattern.

...fileset/src/test/java/org/apache/gravitino/catalog/fileset/TestFilesetCatalogOperations.java

Copilot · 2025-12-05T03:50:39Z

...fileset/src/test/java/org/apache/gravitino/catalog/fileset/TestFilesetCatalogOperations.java

+      try {
+        filesetCatalogOperations.getFileSystem(new Path("file:///tmp"), ImmutableMap.of());
+      } catch (IOException e) {
+        long timeTake = System.currentTimeMillis() - now;
+        Assertions.assertTrue(timeTake <= 10000);
+      }


The test expects an IOException to be thrown but doesn't verify the exception message or type. Consider adding assertions to verify that the correct exception is thrown with an appropriate error message about the timeout, to ensure the error handling path works as expected.

jerryshao · 2025-12-05T03:55:39Z

...log-fileset/src/main/java/org/apache/gravitino/catalog/fileset/FilesetCatalogOperations.java


+  private final ThreadPoolExecutor fileSystemExecutor =
+      new ThreadPoolExecutor(
+          Math.max(2, Math.min(Runtime.getRuntime().availableProcessors() * 2, 16)),


Do we need to keep such threads active for use? My feeling is that most of the threads can be swept when idle to save resources.

I have allowed core poll timeout, and the threads will stop if there is no task assigned to the pool.

jerryshao · 2025-12-05T03:58:38Z

...log-fileset/src/main/java/org/apache/gravitino/catalog/fileset/FilesetCatalogOperations.java

          e);
+    } catch (InterruptedException e) {
+      Thread.currentThread().interrupt();
+      throw new IOException("Interrupted while waiting for FileSystem", e);


I think this is expected, shall we throw an exception here?

Normally, a TimeoutException will occur if it hangs for a long time. and only when we interrupt it deliberately will it throw InterruptedException.

I think you don't understand what I mean. InterruptedException often happens when closing or shutting down. This is expected and should not throw an IOException instead.

...t/java/org/apache/gravitino/catalog/fileset/integration/test/HadoopUserAuthenticationIT.java

jerryshao · 2025-12-05T04:08:35Z

...fileset/src/test/java/org/apache/gravitino/catalog/fileset/TestFilesetCatalogOperations.java

+        filesetCatalogOperations.getFileSystem(new Path("file:///tmp"), ImmutableMap.of());
+      } catch (IOException e) {
+        long timeTake = System.currentTimeMillis() - now;
+        Assertions.assertTrue(timeTake <= 10000);


I guess this will be very flaky if the test machine is under heavy load.

I have used annotation Timeout(15) to replace it.

jerryshao · 2025-12-05T04:10:32Z

...doop-common/src/main/java/org/apache/gravitino/catalog/hadoop/fs/HDFSFileSystemProvider.java

+    }
+
+    if (!configs.containsKey(HDFS_IPC_PING_KEY)) {
+      additionalConfigs.put(HDFS_IPC_PING_KEY, "true");


I found that you have several customized conf values here and above, you'd also define constants for these.

yuqi1129 added 3 commits November 27, 2025 16:27

fix code style

c66f44b

fix

a0d801d

fix

19034c5

yuqi1129 changed the title ~~[#9280] improvement(catalogs-fileset): Refactor FileSystem retrieval to use CompletableFuture~~ [#9280] improvement(catalogs-fileset): Refactor FileSystem retrieval to use CompletableFuture and solve hang problem Nov 27, 2025

yuqi1129 requested a review from Copilot November 28, 2025 03:17

Copilot started reviewing on behalf of yuqi1129 November 28, 2025 03:17 View session

Copilot finished reviewing on behalf of yuqi1129 November 28, 2025 03:19

Copilot AI reviewed Nov 28, 2025

View reviewed changes

yuqi1129 added 3 commits November 28, 2025 16:12

Merge branch 'main' of github.com:datastrato/graviton into issue_9280

709cd1f

Adjust thread pool timeout and improve shutdown handling

fa0dd16

Reduced thread pool keep-alive time from 50ms to 5s for better resource management. Added proper shutdown for the executor and improved logging for filesystem retrieval timeouts to enhance debugging and reliability.

fix

aa80420

yuqi1129 changed the title ~~[#9280] improvement(catalogs-fileset): Refactor FileSystem retrieval to use CompletableFuture and solve hang problem~~ [#9280] improvement(catalogs-fileset): Refactor FileSystem retrieval to use future and solve hang problem Nov 28, 2025

fix

3b55548

yuqi1129 requested review from mchades and roryqi November 28, 2025 09:05

yuqi1129 self-assigned this Nov 28, 2025

yuqi1129 requested a review from jerryshao December 2, 2025 08:03

jerryshao requested a review from Copilot December 3, 2025 02:14

Copilot started reviewing on behalf of jerryshao December 3, 2025 02:14 View session

Copilot finished reviewing on behalf of jerryshao December 3, 2025 02:16

Copilot AI reviewed Dec 3, 2025

View reviewed changes

yuqi1129 added 3 commits December 3, 2025 17:14

test: add timeout test for getFileSystem method in FilesetCatalogOper…

d087ae8

…ations

Merge branch 'main' of github.com:apache/gravitino into issue_9280

5d1c2c9

fix

adc2338

jerryshao reviewed Dec 4, 2025

View reviewed changes

bundles/aliyun/src/main/java/org/apache/gravitino/oss/fs/OSSFileSystemProvider.java Show resolved Hide resolved

jerryshao reviewed Dec 4, 2025

View reviewed changes

bundles/aliyun/src/main/java/org/apache/gravitino/oss/fs/OSSFileSystemProvider.java Show resolved Hide resolved

jerryshao reviewed Dec 4, 2025

View reviewed changes

...fileset/src/test/java/org/apache/gravitino/catalog/fileset/TestFilesetCatalogOperations.java Outdated Show resolved Hide resolved

Copilot AI reviewed Dec 4, 2025

View reviewed changes

fix comments

e5266ab

yuqi1129 closed this Dec 4, 2025

yuqi1129 reopened this Dec 4, 2025

yuqi1129 added 3 commits December 4, 2025 19:33

Merge branch 'main' of github.com:datastrato/graviton into issue_9280

2ac616f

Fix

a263cd1

Merge branch 'main' of github.com:datastrato/graviton into issue_9280

9e9ebaa

yuqi1129 force-pushed the issue_9280 branch from e5266ab to 9e9ebaa Compare December 4, 2025 11:53

yuqi1129 added 3 commits December 4, 2025 20:18

Merge branch 'issue_9280_bak' into issue_9280

6da4673

Fix test error

146bee3

Fix test error

bef0706

yuqi1129 force-pushed the issue_9280 branch from bef0706 to 9e9ebaa Compare December 4, 2025 13:43

yuqi1129 added 5 commits December 4, 2025 22:18

add test

5821c28

test

87107ea

fix

86510d0

fix

8c75fbd

Merge branch 'issue_9280_bak' into issue_9280

1400d93

jerryshao requested a review from Copilot December 5, 2025 03:40

Copilot started reviewing on behalf of jerryshao December 5, 2025 03:40 View session

Copilot finished reviewing on behalf of jerryshao December 5, 2025 03:42

Copilot AI reviewed Dec 5, 2025

View reviewed changes

jerryshao reviewed Dec 5, 2025

View reviewed changes

...t/java/org/apache/gravitino/catalog/fileset/integration/test/HadoopUserAuthenticationIT.java Outdated Show resolved Hide resolved

jerryshao reviewed Dec 5, 2025

View reviewed changes

yuqi1129 added 2 commits December 5, 2025 14:16

fix all comments

8641411

Fix handling InterruptException

276bcc4

jerryshao approved these changes Dec 8, 2025

View reviewed changes

jerryshao merged commit af47d3a into apache:main Dec 8, 2025
26 checks passed

Conversation

yuqi1129 commented Nov 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

yuqi1129 commented Nov 27, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

yuqi1129 commented Nov 27, 2025 •

edited

Loading