Copilot commented on code in PR #9282:
URL: https://github.com/apache/gravitino/pull/9282#discussion_r2591311749
##########
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, 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);
+ }
+ };
Review Comment:
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.
##########
catalogs/catalog-fileset/src/test/java/org/apache/gravitino/catalog/fileset/TestFilesetCatalogOperations.java:
##########
@@ -1868,6 +1874,42 @@ public void testGetTargetLocation() throws IOException {
}
}
+ @Test
+ @Timeout(15)
+ void testGetFileSystemTimeoutThrowsException() throws Exception {
+ FieldUtils.writeField(
+ GravitinoEnv.getInstance(), "entityStore", new
RelationalEntityStore(), true);
+
+ try (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 -> {
+ // Block 100s, however, the timeout is set to 6s by default in
+ // FilesetCatalogOperations, so it's expected to be over
within 10s
+ Awaitility.await().forever().until(() -> false);
+ 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) {
+ long timeTake = System.currentTimeMillis() - now;
+ Assertions.assertTrue(timeTake <= 10000);
+ }
Review Comment:
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.
##########
catalogs/catalog-fileset/src/test/java/org/apache/gravitino/catalog/fileset/TestFilesetCatalogOperations.java:
##########
@@ -1868,6 +1874,42 @@ public void testGetTargetLocation() throws IOException {
}
}
+ @Test
+ @Timeout(15)
+ void testGetFileSystemTimeoutThrowsException() throws Exception {
+ FieldUtils.writeField(
+ GravitinoEnv.getInstance(), "entityStore", new
RelationalEntityStore(), true);
+
+ try (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 -> {
+ // Block 100s, however, the timeout is set to 6s by default in
+ // FilesetCatalogOperations, so it's expected to be over
within 10s
+ Awaitility.await().forever().until(() -> false);
Review Comment:
The test uses `Awaitility.await().forever().until(() -> false)` to simulate
a blocking call. However, this creates a busy-wait loop that consumes CPU
unnecessarily. Consider using `Thread.sleep(Long.MAX_VALUE)` or
`CountDownLatch.await()` instead, which would be more efficient and clearer in
intent.
```suggestion
Thread.sleep(Long.MAX_VALUE);
```
##########
bundles/gcp/src/main/java/org/apache/gravitino/gcs/fs/GCSFileSystemProvider.java:
##########
@@ -71,4 +77,30 @@ public String scheme() {
public String name() {
return "gcs";
}
+
+ /**
+ * Add additional GCS specific configuration to tune the performance.
+ *
+ * @param configs original configuration
+ * @return the configuration with additional GCS tuning parameters
+ */
+ 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(GCS_GCS_HTTP_CONNECT_TIMEOUT_KEY)) {
+ //
Review Comment:
The comment on line 94 is incomplete - it starts with "//" but has no
content. This should either be removed or completed with meaningful
documentation.
```suggestion
```
##########
catalogs/catalog-fileset/src/test/java/org/apache/gravitino/catalog/fileset/TestFilesetCatalogOperations.java:
##########
@@ -1868,6 +1874,42 @@ public void testGetTargetLocation() throws IOException {
}
}
+ @Test
+ @Timeout(15)
+ void testGetFileSystemTimeoutThrowsException() throws Exception {
+ FieldUtils.writeField(
+ GravitinoEnv.getInstance(), "entityStore", new
RelationalEntityStore(), true);
+
+ try (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 -> {
+ // Block 100s, however, the timeout is set to 6s by default in
+ // FilesetCatalogOperations, so it's expected to be over
within 10s
+ Awaitility.await().forever().until(() -> false);
+ 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) {
+ long timeTake = System.currentTimeMillis() - now;
+ Assertions.assertTrue(timeTake <= 10000);
+ }
Review Comment:
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.
```suggestion
// 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");
}
});
```
--
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]
