Copilot commented on code in PR #9282:
URL: https://github.com/apache/gravitino/pull/9282#discussion_r2583408603
##########
bundles/aws/src/main/java/org/apache/gravitino/s3/fs/S3FileSystemProvider.java:
##########
@@ -129,6 +131,33 @@ private void checkAndSetCredentialProvider(Map<String,
String> configs) {
}
}
+ private Map<String, String> additionalS3Config(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 Constants.MAX_ERROR_RETRIES to avoid
dependency on a specific Hadoop
+ // version
+ if (!configs.containsKey("fs.s3a.attempts.maximum")) {
+ additionalConfigs.put("fs.s3a.attempts.maximum", "2");
+ }
+
+ if (!configs.containsKey("fs.s3a.connection.establish.timeout")) {
+ additionalConfigs.put("fs.s3a.connection.establish.timeout", "5000");
+ }
+
+ if (!configs.containsKey("fs.s3a.retry.limit")) {
+ additionalConfigs.put("fs.s3a.retry.limit", "2");
+ }
+
+ if (!configs.containsKey("fs.s3a.retry.throttle.limit")) {
+ additionalConfigs.put("fs.s3a.retry.throttle.limit", "2");
+ }
+
+ // More tuning can be added here.
+
+ return ImmutableMap.copyOf(additionalConfigs);
+ }
Review Comment:
The `additionalS3Config` method lacks test coverage. Consider adding tests
to verify:
1. Default timeout and retry values are applied when not present in the
config
2. User-provided values are preserved and not overridden
3. The returned map is immutable (ImmutableMap behavior)
##########
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;
}
+ private Map<String, String> additionalAzureConfig(Map<String, String>
configs) {
+ Map<String, String> additionalConfigs = Maps.newHashMap(configs);
+
+ // Avoid multiple retries to speed up failure in test cases.
+ if (!configs.containsKey("fs.azure.io.retry.max.retries")) {
+ additionalConfigs.put("fs.azure.io.retry.max.retries", "2");
+ }
+
+ // More tuning can be added here.
+
+ return ImmutableMap.copyOf(additionalConfigs);
+ }
Review Comment:
The `additionalAzureConfig` method lacks test coverage. Consider adding
tests to verify:
1. Default retry values are applied when not present in the config
2. User-provided values are preserved and not overridden
3. The returned map is immutable (ImmutableMap behavior)
##########
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;
}
+ private Map<String, String> additionalGCSConfig(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
GoogleHadoopFileSystemBase.GCS_HTTP_CONNECT_TIMEOUT_KEY to avoid
+ // dependency on a specific Hadoop version
+ if (!configs.containsKey("fs.gs.http.connect-timeout")) {
+ additionalConfigs.put("fs.gs.http.connect-timeout", "5000");
+ }
+
+ if (!configs.containsKey("fs.gs.http.max.retry")) {
+ additionalConfigs.put("fs.gs.http.max.retry", "2");
+ }
+
+ // More tuning can be added here.
+
+ return ImmutableMap.copyOf(additionalConfigs);
+ }
Review Comment:
The `additionalGCSConfig` method lacks test coverage. Consider adding tests
to verify:
1. Default timeout and retry values are applied when not present in the
config
2. User-provided values are preserved and not overridden
3. The returned map is immutable (ImmutableMap behavior)
##########
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;
}
+ private Map<String, String> additionalOSSConfig(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 Constants.ESTABLISH_TIMEOUT_KEY to avoid
dependency on a specific
+ // Hadoop version.
+ if (!configs.containsKey("fs.oss.connection.establish.timeout")) {
+ additionalConfigs.put("fs.oss.connection.establish.timeout", "5000");
+ }
+
+ if (!configs.containsKey("fs.oss.attempts.maximum")) {
+ additionalConfigs.put("fs.oss.attempts.maximum", "2");
+ }
+
+ // More tuning can be added here.
+
+ return ImmutableMap.copyOf(additionalConfigs);
+ }
Review Comment:
The `additionalOSSConfig` method lacks test coverage. Consider adding tests
to verify:
1. Default timeout and retry values are applied when not present in the
config
2. User-provided values are preserved and not overridden
3. The returned map is immutable (ImmutableMap behavior)
##########
catalogs/catalog-fileset/src/main/java/org/apache/gravitino/catalog/fileset/FilesetCatalogOperations.java:
##########
@@ -1397,30 +1419,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 "
+ "seconds, please check the configuration or increase the "
+ "file system connection timeout time by setting catalog
property: %s",
path,
scheme,
provider,
- config,
timeoutSeconds,
FilesetCatalogPropertiesMetadata.FILESYSTEM_CONNECTION_TIMEOUT_SECONDS),
e);
+ } catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
+ throw new IOException("Interrupted while waiting for FileSystem", e);
+ } catch (ExecutionException e) {
+ Throwable cause = e.getCause();
+ if (cause instanceof IOException) {
+ throw (IOException) cause;
+ }
+ throw new IOException("Failed to create FileSystem", cause);
}
Review Comment:
The new timeout handling mechanism using `Future` and the
`fileSystemExecutor` thread pool lacks test coverage. No tests verify:
1. The timeout behavior when FileSystem creation hangs
2. The cancellation of hung tasks via `fileSystemFuture.cancel(true)`
3. The various exception paths (TimeoutException, InterruptedException,
ExecutionException)
4. The behavior when the executor's queue is full (AbortPolicy)
Consider adding tests that simulate these scenarios, especially the hang
condition that this PR is designed to fix.
##########
catalogs/hadoop-common/src/main/java/org/apache/gravitino/catalog/hadoop/fs/HDFSFileSystemProvider.java:
##########
@@ -41,6 +45,25 @@ public String scheme() {
return "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");
+ }
+
+ if (!configs.containsKey("ipc.client.ping")) {
+ additionalConfigs.put("ipc.client.ping", "true");
+ }
+
+ // More tuning can be added here.
+
+ return ImmutableMap.copyOf(additionalConfigs);
+ }
Review Comment:
The `additionalHDFSConfig` method lacks test coverage. Consider adding tests
to verify:
1. Default timeout values are applied when not present in the config
2. User-provided timeout values are preserved and not overridden
3. The returned map is immutable (ImmutableMap behavior)
--
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]
