Copilot commented on code in PR #9282:
URL: https://github.com/apache/gravitino/pull/9282#discussion_r2587894164
##########
catalogs/catalog-fileset/src/test/java/org/apache/gravitino/catalog/fileset/TestFilesetCatalogOperations.java:
##########
@@ -1868,6 +1872,41 @@ public void testGetTargetLocation() throws IOException {
}
}
+ @Test
+ void testGetFileSystemWithTimeout() throws Exception {
+ FieldUtils.writeField(
+ GravitinoEnv.getInstance(), "entityStore", new
RelationalEntityStore(), true);
+
+ FilesetCatalogOperations filesetCatalogOperations = new
FilesetCatalogOperations();
+
+ LocalFileSystemProvider localFileSystemProvider =
Mockito.mock(LocalFileSystemProvider.class);
+ when(localFileSystemProvider.scheme()).thenReturn("file");
+ when(localFileSystemProvider.getFileSystem(Mockito.any(Path.class),
Mockito.anyMap()))
Review Comment:
Using `Mockito.any(Path.class)` and `Mockito.anyMap()` instead of the
imported `any()` matcher is inconsistent. For consistency with the changes on
lines 1462 and 1464 which use `any()`, these should also use `any()`:
```java
when(localFileSystemProvider.getFileSystem(any(Path.class), anyMap()))
```
```suggestion
when(localFileSystemProvider.getFileSystem(any(), any()))
```
##########
catalogs/catalog-fileset/src/test/java/org/apache/gravitino/catalog/fileset/TestFilesetCatalogOperations.java:
##########
@@ -1868,6 +1872,41 @@ public void testGetTargetLocation() throws IOException {
}
}
+ @Test
+ void testGetFileSystemWithTimeout() throws Exception {
+ FieldUtils.writeField(
+ GravitinoEnv.getInstance(), "entityStore", new
RelationalEntityStore(), true);
+
+ FilesetCatalogOperations filesetCatalogOperations = new
FilesetCatalogOperations();
+
+ LocalFileSystemProvider localFileSystemProvider =
Mockito.mock(LocalFileSystemProvider.class);
+ when(localFileSystemProvider.scheme()).thenReturn("file");
+ when(localFileSystemProvider.getFileSystem(Mockito.any(Path.class),
Mockito.anyMap()))
+ .thenAnswer(
+ invocation -> {
+ // Sleep 100s, however, the timeout is set to 6s by default in
+ // FilesetCatalogOperations, so
+ // it's expected to over within 10s
+ Thread.sleep(100000); // Simulate delay
+ return new LocalFileSystem();
+ });
+ Map<String, FileSystemProvider> fileSystemProviderMapOriginal = new
HashMap<>();
+ fileSystemProviderMapOriginal.put("file", localFileSystemProvider);
+ FieldUtils.writeField(
+ filesetCatalogOperations, "fileSystemProvidersMap",
fileSystemProviderMapOriginal, true);
+
+ FieldUtils.writeField(
+ filesetCatalogOperations, "propertiesMetadata",
FILESET_PROPERTIES_METADATA, true);
+
+ // Test the following method should finish with 10s
+ long now = System.currentTimeMillis();
+ try {
+ filesetCatalogOperations.getFileSystem(new Path("file:///tmp"),
ImmutableMap.of());
+ } catch (IOException e) {
+ Assertions.assertTrue(System.currentTimeMillis() - now <= 10000);
+ }
+ }
Review Comment:
The test should call `filesetCatalogOperations.close()` to clean up the
thread pool created in the test. Without this, the `fileSystemExecutor` threads
will remain running after the test completes, potentially causing resource
leaks or interference with other tests.
Add cleanup after the assertion:
```java
} finally {
filesetCatalogOperations.close();
}
```
```suggestion
try {
LocalFileSystemProvider localFileSystemProvider =
Mockito.mock(LocalFileSystemProvider.class);
when(localFileSystemProvider.scheme()).thenReturn("file");
when(localFileSystemProvider.getFileSystem(Mockito.any(Path.class),
Mockito.anyMap()))
.thenAnswer(
invocation -> {
// Sleep 100s, however, the timeout is set to 6s by default
in
// FilesetCatalogOperations, so
// it's expected to over within 10s
Thread.sleep(100000); // Simulate delay
return new LocalFileSystem();
});
Map<String, FileSystemProvider> fileSystemProviderMapOriginal = new
HashMap<>();
fileSystemProviderMapOriginal.put("file", localFileSystemProvider);
FieldUtils.writeField(
filesetCatalogOperations, "fileSystemProvidersMap",
fileSystemProviderMapOriginal, true);
FieldUtils.writeField(
filesetCatalogOperations, "propertiesMetadata",
FILESET_PROPERTIES_METADATA, true);
// Test the following method should finish with 10s
long now = System.currentTimeMillis();
try {
filesetCatalogOperations.getFileSystem(new Path("file:///tmp"),
ImmutableMap.of());
} catch (IOException e) {
Assertions.assertTrue(System.currentTimeMillis() - now <= 10000);
}
} finally {
filesetCatalogOperations.close();
}
```
##########
catalogs/catalog-fileset/src/main/java/org/apache/gravitino/catalog/fileset/FilesetCatalogOperations.java:
##########
@@ -994,6 +1014,8 @@ public void close() throws IOException {
scheduler.shutdownNow();
}
+ fileSystemExecutor.shutdownNow();
Review Comment:
The `fileSystemExecutor.shutdownNow()` call should wait for termination and
handle interruption of running tasks properly. Currently, it immediately shuts
down without waiting, which could lead to tasks being interrupted abruptly.
Consider adding:
```java
fileSystemExecutor.shutdownNow();
try {
if (!fileSystemExecutor.awaitTermination(5, TimeUnit.SECONDS)) {
LOG.warn("FileSystem executor did not terminate in time");
}
} catch (InterruptedException e) {
Thread.currentThread().interrupt();
LOG.warn("Interrupted while waiting for executor shutdown", e);
}
```
This ensures a more graceful shutdown and proper handling of interrupted
threads.
```suggestion
fileSystemExecutor.shutdownNow();
try {
if (!fileSystemExecutor.awaitTermination(5, TimeUnit.SECONDS)) {
LOG.warn("FileSystem executor did not terminate in time");
}
} catch (InterruptedException e) {
Thread.currentThread().interrupt();
LOG.warn("Interrupted while waiting for executor shutdown", e);
}
```
##########
catalogs/catalog-fileset/src/test/java/org/apache/gravitino/catalog/fileset/TestFilesetCatalogOperations.java:
##########
@@ -1868,6 +1872,41 @@ public void testGetTargetLocation() throws IOException {
}
}
+ @Test
+ void testGetFileSystemWithTimeout() throws Exception {
+ FieldUtils.writeField(
+ GravitinoEnv.getInstance(), "entityStore", new
RelationalEntityStore(), true);
+
+ FilesetCatalogOperations filesetCatalogOperations = new
FilesetCatalogOperations();
+
+ LocalFileSystemProvider localFileSystemProvider =
Mockito.mock(LocalFileSystemProvider.class);
+ when(localFileSystemProvider.scheme()).thenReturn("file");
+ when(localFileSystemProvider.getFileSystem(Mockito.any(Path.class),
Mockito.anyMap()))
+ .thenAnswer(
+ invocation -> {
+ // Sleep 100s, however, the timeout is set to 6s by default in
+ // FilesetCatalogOperations, so
+ // it's expected to over within 10s
Review Comment:
The comment states "it's expected to over within 10s" but should be "it's
expected to be over within 10s".
```suggestion
// it's expected to be over within 10s
```
##########
catalogs/catalog-fileset/src/test/java/org/apache/gravitino/catalog/fileset/TestFilesetCatalogOperations.java:
##########
@@ -1868,6 +1872,41 @@ public void testGetTargetLocation() throws IOException {
}
}
+ @Test
+ void testGetFileSystemWithTimeout() throws Exception {
Review Comment:
[nitpick] The test name `testGetFileSystemWithTimeout` doesn't clearly
indicate that it's testing a timeout failure scenario. Consider renaming to
`testGetFileSystemTimeoutThrowsException` or `testGetFileSystemHandlesTimeout`
to make the test intention clearer.
```suggestion
void testGetFileSystemTimeoutThrowsException() throws Exception {
```
##########
catalogs/catalog-fileset/src/main/java/org/apache/gravitino/catalog/fileset/FilesetCatalogOperations.java:
##########
@@ -137,6 +140,23 @@ public class FilesetCatalogOperations extends
ManagedSchemaOperations
@VisibleForTesting ScheduledThreadPoolExecutor scheduler;
@VisibleForTesting Cache<FileSystemCacheKey, FileSystem> fileSystemCache;
+ private final ThreadPoolExecutor fileSystemExecutor =
+ new ThreadPoolExecutor(
+ Math.max(2, Runtime.getRuntime().availableProcessors() * 2),
+ Math.max(2, Runtime.getRuntime().availableProcessors() * 2),
+ 5L,
+ TimeUnit.SECONDS,
+ new ArrayBlockingQueue<>(1000),
+ new ThreadFactoryBuilder()
+ .setDaemon(true)
+ .setNameFormat("fileset-filesystem-getter-pool-%d")
+ .build(),
+ new ThreadPoolExecutor.AbortPolicy()) {
+ {
+ allowCoreThreadTimeOut(true);
+ }
+ };
Review Comment:
The `fileSystemExecutor` ThreadPoolExecutor is initialized as a final field
during class construction. This means it's created immediately when the class
is instantiated, even before `initialize()` is called. This can lead to
resource leaks if the class is instantiated but never properly initialized or
closed.
Consider making this a lazy-initialized field that is created in the
`initialize()` method, similar to how `scheduler` is handled. This ensures
proper lifecycle management and prevents creating thread pools for objects that
may not be fully initialized.
##########
bundles/aws/src/main/java/org/apache/gravitino/s3/fs/S3FileSystemProvider.java:
##########
@@ -129,6 +131,33 @@ private void checkAndSetCredentialProvider(Map<String,
String> configs) {
}
}
Review Comment:
The `additionalS3Config` method lacks proper documentation. Since this
method modifies configuration with performance-impacting defaults, it should
have a Javadoc explaining the purpose and impact, similar to the other
filesystem providers.
Add Javadoc:
```java
/**
* Adds additional S3 configuration defaults to speed up failure detection.
* These configurations reduce retry attempts and timeouts to prevent long
hangs
* when the S3 service is unreachable.
*
* @param configs The original configuration map
* @return A new immutable map with additional configurations
*/
private Map<String, String> additionalS3Config(Map<String, String> configs) {
```
```suggestion
/**
* Adds additional S3 configuration defaults to speed up failure detection.
* <p>
* These configurations reduce retry attempts and timeouts to prevent long
hangs
* when the S3 service is unreachable. This is especially useful in test
cases
* and environments where fast failure is preferred over long retry cycles.
* <p>
* The following defaults are set if not already present:
* <ul>
* <li>fs.s3a.attempts.maximum = 2</li>
* <li>fs.s3a.connection.establish.timeout = 5000</li>
* <li>fs.s3a.retry.limit = 2</li>
* <li>fs.s3a.retry.throttle.limit = 2</li>
* </ul>
*
* @param configs The original configuration map
* @return A new immutable map with additional configurations
*/
```
##########
bundles/azure/src/main/java/org/apache/gravitino/abs/fs/AzureFileSystemProvider.java:
##########
@@ -101,6 +103,19 @@ public Map<String, String>
getFileSystemCredentialConf(Credential[] credentials)
return result;
}
Review Comment:
The `additionalAzureConfig` method lacks proper documentation. Since this
method modifies configuration with performance-impacting defaults, it should
have a Javadoc explaining the purpose and impact, similar to the other
filesystem providers.
Add Javadoc:
```java
/**
* Adds additional Azure configuration defaults to speed up failure
detection.
* These configurations reduce retry attempts to prevent long hangs
* when the Azure service is unreachable.
*
* @param configs The original configuration map
* @return A new immutable map with additional configurations
*/
private Map<String, String> additionalAzureConfig(Map<String, String>
configs) {
```
```suggestion
/**
* Adds additional Azure configuration defaults to speed up failure
detection.
* These configurations reduce retry attempts to prevent long hangs
* when the Azure service is unreachable.
*
* @param configs The original configuration map
* @return A new immutable map with additional configurations
*/
```
##########
catalogs/catalog-fileset/src/test/java/org/apache/gravitino/catalog/fileset/TestFilesetCatalogOperations.java:
##########
@@ -1868,6 +1872,41 @@ public void testGetTargetLocation() throws IOException {
}
}
+ @Test
+ void testGetFileSystemWithTimeout() throws Exception {
+ FieldUtils.writeField(
+ GravitinoEnv.getInstance(), "entityStore", new
RelationalEntityStore(), true);
+
+ FilesetCatalogOperations filesetCatalogOperations = new
FilesetCatalogOperations();
+
+ LocalFileSystemProvider localFileSystemProvider =
Mockito.mock(LocalFileSystemProvider.class);
+ when(localFileSystemProvider.scheme()).thenReturn("file");
+ when(localFileSystemProvider.getFileSystem(Mockito.any(Path.class),
Mockito.anyMap()))
+ .thenAnswer(
+ invocation -> {
+ // Sleep 100s, however, the timeout is set to 6s by default in
+ // FilesetCatalogOperations, so
+ // it's expected to over within 10s
+ Thread.sleep(100000); // Simulate delay
+ return new LocalFileSystem();
+ });
+ Map<String, FileSystemProvider> fileSystemProviderMapOriginal = new
HashMap<>();
+ fileSystemProviderMapOriginal.put("file", localFileSystemProvider);
+ FieldUtils.writeField(
+ filesetCatalogOperations, "fileSystemProvidersMap",
fileSystemProviderMapOriginal, true);
Review Comment:
[nitpick] The variable name `fileSystemProviderMapOriginal` suggests it's
the original map, but it's actually a newly created map for testing purposes.
Consider renaming to `testFileSystemProviderMap` or `mockFileSystemProviderMap`
for clarity.
```suggestion
Map<String, FileSystemProvider> testFileSystemProviderMap = new
HashMap<>();
testFileSystemProviderMap.put("file", localFileSystemProvider);
FieldUtils.writeField(
filesetCatalogOperations, "fileSystemProvidersMap",
testFileSystemProviderMap, true);
```
##########
bundles/gcp/src/main/java/org/apache/gravitino/gcs/fs/GCSFileSystemProvider.java:
##########
@@ -62,6 +64,25 @@ public Map<String, String>
getFileSystemCredentialConf(Credential[] credentials)
return result;
}
Review Comment:
The `additionalGCSConfig` method lacks proper documentation. Since this
method modifies configuration with performance-impacting defaults, it should
have a Javadoc explaining the purpose and impact, similar to the other
filesystem providers.
Add Javadoc:
```java
/**
* Adds additional GCS configuration defaults to speed up failure detection.
* These configurations reduce retry attempts and timeouts to prevent long
hangs
* when the GCS service is unreachable.
*
* @param configs The original configuration map
* @return A new immutable map with additional configurations
*/
private Map<String, String> additionalGCSConfig(Map<String, String> configs)
{
```
```suggestion
/**
* Adds additional GCS configuration defaults to speed up failure
detection.
* These configurations reduce retry attempts and timeouts to prevent long
hangs
* when the GCS service is unreachable.
*
* @param configs The original configuration map
* @return A new immutable map with additional configurations
*/
```
##########
catalogs/catalog-fileset/src/test/java/org/apache/gravitino/catalog/fileset/TestFilesetCatalogOperations.java:
##########
@@ -1868,6 +1872,41 @@ public void testGetTargetLocation() throws IOException {
}
}
+ @Test
+ void testGetFileSystemWithTimeout() throws Exception {
+ FieldUtils.writeField(
+ GravitinoEnv.getInstance(), "entityStore", new
RelationalEntityStore(), true);
+
+ FilesetCatalogOperations filesetCatalogOperations = new
FilesetCatalogOperations();
+
+ LocalFileSystemProvider localFileSystemProvider =
Mockito.mock(LocalFileSystemProvider.class);
+ when(localFileSystemProvider.scheme()).thenReturn("file");
+ when(localFileSystemProvider.getFileSystem(Mockito.any(Path.class),
Mockito.anyMap()))
+ .thenAnswer(
+ invocation -> {
+ // Sleep 100s, however, the timeout is set to 6s by default in
+ // FilesetCatalogOperations, so
+ // it's expected to over within 10s
+ Thread.sleep(100000); // Simulate delay
+ return new LocalFileSystem();
+ });
+ Map<String, FileSystemProvider> fileSystemProviderMapOriginal = new
HashMap<>();
+ fileSystemProviderMapOriginal.put("file", localFileSystemProvider);
+ FieldUtils.writeField(
+ filesetCatalogOperations, "fileSystemProvidersMap",
fileSystemProviderMapOriginal, true);
+
+ FieldUtils.writeField(
+ filesetCatalogOperations, "propertiesMetadata",
FILESET_PROPERTIES_METADATA, true);
+
+ // Test the following method should finish with 10s
+ long now = System.currentTimeMillis();
+ try {
+ filesetCatalogOperations.getFileSystem(new Path("file:///tmp"),
ImmutableMap.of());
+ } catch (IOException e) {
+ Assertions.assertTrue(System.currentTimeMillis() - now <= 10000);
+ }
Review Comment:
The timeout assertion logic is incorrect. If no exception is thrown (which
would be a bug), this test would pass incorrectly. The assertion should be
outside the catch block to ensure the method actually throws an exception.
Refactor to:
```java
long now = System.currentTimeMillis();
IOException exception = Assertions.assertThrows(
IOException.class,
() -> filesetCatalogOperations.getFileSystem(new Path("file:///tmp"),
ImmutableMap.of()));
Assertions.assertTrue(System.currentTimeMillis() - now <= 10000);
```
This ensures that an IOException is thrown and the timeout is enforced
correctly.
```suggestion
IOException exception = Assertions.assertThrows(
IOException.class,
() -> filesetCatalogOperations.getFileSystem(new
Path("file:///tmp"), ImmutableMap.of()));
Assertions.assertTrue(System.currentTimeMillis() - now <= 10000);
```
##########
catalogs/hadoop-common/src/main/java/org/apache/gravitino/catalog/hadoop/fs/HDFSFileSystemProvider.java:
##########
@@ -44,6 +48,25 @@ public String scheme() {
return SCHEME_HDFS;
}
+ private Map<String, String> additionalHDFSConfig(Map<String, String>
configs) {
+ Map<String, String> additionalConfigs = Maps.newHashMap(configs);
+
+ // Avoid multiple retries to speed up failure in test cases.
+ // Use hard code instead of
CommonConfigurationKeysPublic.IPC_CLIENT_CONNECT_TIMEOUT_KEY to
+ // avoid dependency on a specific Hadoop version.
+ if (!configs.containsKey("ipc.client.connect.timeout")) {
+ additionalConfigs.put("ipc.client.connect.timeout", "5000");
Review Comment:
[nitpick] The hardcoded timeout value of 5000ms should be documented or made
a constant. This magic number appears across multiple filesystem providers
(HDFS, GCS, S3, Azure, OSS) with the same value. Consider extracting this to a
shared constant for consistency and maintainability.
For example:
```java
private static final String DEFAULT_CONNECTION_TIMEOUT = "5000";
```
This would make it easier to adjust these values globally if needed and
makes the intent clearer.
##########
catalogs/hadoop-common/src/main/java/org/apache/gravitino/catalog/hadoop/fs/HDFSFileSystemProvider.java:
##########
@@ -44,6 +48,25 @@ public String scheme() {
return SCHEME_HDFS;
}
Review Comment:
The `additionalHDFSConfig` method lacks proper documentation. Since this
method modifies configuration with performance-impacting defaults, it should
have a Javadoc explaining:
- The purpose of adding these configurations
- Why these specific defaults are chosen
- The impact on behavior (faster failures in test cases)
Example:
```java
/**
* Adds additional HDFS configuration defaults to speed up failure detection.
* These configurations reduce retry attempts and timeouts to prevent long
hangs
* when the HDFS service is unreachable.
*
* @param configs The original configuration map
* @return A new immutable map with additional configurations
*/
private Map<String, String> additionalHDFSConfig(Map<String, String>
configs) {
```
```suggestion
/**
* Adds additional HDFS configuration defaults to speed up failure
detection.
* <p>
* These configurations reduce retry attempts and timeouts to prevent long
hangs
* when the HDFS service is unreachable, especially in test cases.
Hardcoded keys
* are used instead of Hadoop constants to avoid dependency on a specific
Hadoop version.
* <ul>
* <li>ipc.client.connect.timeout: Set to 5000ms if not present, to
reduce connection wait time.</li>
* <li>ipc.client.ping: Set to true if not present, to enable client
pinging.</li>
* </ul>
*
* @param configs The original configuration map
* @return A new immutable map with additional configurations for faster
failure detection
*/
```
##########
catalogs/catalog-fileset/src/main/java/org/apache/gravitino/catalog/fileset/FilesetCatalogOperations.java:
##########
@@ -137,6 +140,23 @@ public class FilesetCatalogOperations extends
ManagedSchemaOperations
@VisibleForTesting ScheduledThreadPoolExecutor scheduler;
@VisibleForTesting Cache<FileSystemCacheKey, FileSystem> fileSystemCache;
+ private final ThreadPoolExecutor fileSystemExecutor =
+ new ThreadPoolExecutor(
+ Math.max(2, Runtime.getRuntime().availableProcessors() * 2),
+ Math.max(2, Runtime.getRuntime().availableProcessors() * 2),
+ 5L,
+ TimeUnit.SECONDS,
+ new ArrayBlockingQueue<>(1000),
+ new ThreadFactoryBuilder()
+ .setDaemon(true)
+ .setNameFormat("fileset-filesystem-getter-pool-%d")
+ .build(),
+ new ThreadPoolExecutor.AbortPolicy()) {
+ {
+ allowCoreThreadTimeOut(true);
+ }
+ };
Review Comment:
[nitpick] The ThreadPoolExecutor is configured with a fixed core and maximum
pool size (both set to `availableProcessors() * 2`), combined with
`allowCoreThreadTimeOut(true)`. This means all threads can timeout and be
terminated, which could cause thread creation overhead on every FileSystem
retrieval operation.
Consider either:
1. Keep a minimum number of core threads alive (e.g., 1-2) by not allowing
core thread timeout
2. Or increase the keep-alive time from 5 seconds to reduce frequent thread
creation/destruction
For example:
```java
Math.max(2, Runtime.getRuntime().availableProcessors()), // corePoolSize
Math.max(2, Runtime.getRuntime().availableProcessors() * 2), //
maximumPoolSize
60L, // increased keepAliveTime
TimeUnit.SECONDS,
```
This would maintain a more stable thread pool for better performance.
```suggestion
Math.max(2, Runtime.getRuntime().availableProcessors()), //
corePoolSize
Math.max(2, Runtime.getRuntime().availableProcessors() * 2), //
maximumPoolSize
60L, // increased keepAliveTime
TimeUnit.SECONDS,
new ArrayBlockingQueue<>(1000),
new ThreadFactoryBuilder()
.setDaemon(true)
.setNameFormat("fileset-filesystem-getter-pool-%d")
.build(),
new ThreadPoolExecutor.AbortPolicy());
```
##########
catalogs/catalog-fileset/src/main/java/org/apache/gravitino/catalog/fileset/FilesetCatalogOperations.java:
##########
@@ -1395,30 +1417,43 @@ FileSystem getFileSystem(Path path, Map<String, String>
config) throws IOExcepti
.catalogPropertiesMetadata()
.getOrDefault(
config,
FilesetCatalogPropertiesMetadata.FILESYSTEM_CONNECTION_TIMEOUT_SECONDS);
+
+ Future<FileSystem> fileSystemFuture =
+ fileSystemExecutor.submit(() -> provider.getFileSystem(path, config));
+
try {
- AtomicReference<FileSystem> fileSystem = new AtomicReference<>();
- Awaitility.await()
- .atMost(timeoutSeconds, TimeUnit.SECONDS)
- .pollInterval(1, TimeUnit.MILLISECONDS)
- .until(
- () -> {
- fileSystem.set(provider.getFileSystem(path, config));
- return true;
- });
- return fileSystem.get();
- } catch (ConditionTimeoutException e) {
+ return fileSystemFuture.get(timeoutSeconds, TimeUnit.SECONDS);
+ } catch (TimeoutException e) {
+ fileSystemFuture.cancel(true);
+
+ LOG.warn(
+ "Timeout when getting FileSystem for path: {}, scheme: {}, provider:
{} within {} seconds",
+ path,
+ scheme,
+ provider,
+ timeoutSeconds,
+ e);
+
throw new IOException(
String.format(
- "Failed to get FileSystem for path: %s, scheme: %s, provider:
%s, config: %s within %s "
+ "Failed to get FileSystem for path: %s, scheme: %s, provider:
%s, within %s "
Review Comment:
The error message on line 1439 has an extra comma after "provider: %s,"
which results in awkward formatting like "provider: XYZ, within". Remove the
comma:
```java
"Failed to get FileSystem for path: %s, scheme: %s, provider: %s within %s "
```
```suggestion
"Failed to get FileSystem for path: %s, scheme: %s, provider:
%s within %s "
```
##########
bundles/aliyun/src/main/java/org/apache/gravitino/oss/fs/OSSFileSystemProvider.java:
##########
@@ -80,6 +82,25 @@ public Map<String, String>
getFileSystemCredentialConf(Credential[] credentials)
return result;
}
Review Comment:
The `additionalOSSConfig` method lacks proper documentation. Since this
method modifies configuration with performance-impacting defaults, it should
have a Javadoc explaining the purpose and impact, similar to the other
filesystem providers.
Add Javadoc:
```java
/**
* Adds additional OSS configuration defaults to speed up failure detection.
* These configurations reduce retry attempts and timeouts to prevent long
hangs
* when the OSS service is unreachable.
*
* @param configs The original configuration map
* @return A new immutable map with additional configurations
*/
private Map<String, String> additionalOSSConfig(Map<String, String> configs)
{
```
```suggestion
/**
* Adds additional OSS configuration defaults to speed up failure
detection.
* These configurations reduce retry attempts and timeouts to prevent long
hangs
* when the OSS service is unreachable. This is especially useful in test
cases
* and environments where quick failure is preferred over long retries.
*
* @param configs The original configuration map
* @return A new immutable map with additional configurations for OSS
*/
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
