Copilot commented on code in PR #12857:
URL: https://github.com/apache/gravitino/pull/12857#discussion_r3915486192
##########
clients/filesystem-hadoop3/src/main/java/org/apache/gravitino/filesystem/hadoop/BaseGVFSOperations.java:
##########
@@ -962,14 +1072,42 @@ private Cache<FileSystemCacheKey, FileSystem>
newFileSystemCache(Configuration c
return cacheBuilder.build();
}
+ /**
+ * The properties that identify which {@link FileSystem} instance a path
belongs to: the catalog
+ * and fileset properties plus this filesystem's own configuration. Every
value here is already in
+ * hand once the fileset is loaded, so building this costs no server call.
+ *
+ * <p>Kept separate from {@link #getAllProperties} because {@link
+ * FileSystemProvider#getFullAuthority} reads from it to form the filesystem
cache key, which has
+ * to happen before the cache is consulted -- and therefore on every single
operation. Secrets are
+ * deliberately not resolved here: each {@code getSecrets()} is a REST call,
and no provider
+ * derives an authority from a secret.
+ */
+ private Map<String, String> getIdentityProperties(Fileset fileset, Catalog
catalog) {
+ Map<String, String> properties = new HashMap<>();
+ if (catalog.properties() != null) {
+ properties.putAll(catalog.properties());
+ }
+ if (fileset.properties() != null) {
+ properties.putAll(fileset.properties());
+ }
+ properties.putAll(extractNonDefaultConfig(conf));
+ return properties;
+ }
+
+ /**
+ * Everything needed to construct a {@link FileSystem}: the schema's
properties, and the secrets
+ * of the catalog, schema and fileset.
+ *
+ * <p>Loading the schema is a server call when the metadata cache is off,
and each {@code
+ * getSecrets()} is another one that no cache absorbs. The result is only
read when the filesystem
+ * cache misses -- once per scheme, authority and user per JVM. Callers
therefore pass this as a
+ * supplier rather than a value; see {@link #getActualFileSystemByPath}.
+ */
@VisibleForTesting
- Map<String, String> getAllProperties(NameIdentifier filesetIdent) {
- String catalogName = filesetIdent.namespace().level(1);
- String schemaName = filesetIdent.namespace().level(2);
- Catalog catalog = getGravitinoClient().loadCatalog(catalogName);
- Schema schema = catalog.asSchemas().loadSchema(schemaName);
- Fileset fileset =
- catalog.asFilesetCatalog().loadFileset(NameIdentifier.of(schemaName,
filesetIdent.name()));
+ Map<String, String> getAllProperties(
+ NameIdentifier filesetIdent, Fileset fileset, Catalog catalog) {
+ Schema schema =
getSchema(NameIdentifier.parse(filesetIdent.namespace().toString()), catalog);
Review Comment:
`NameIdentifier.parse(filesetIdent.namespace().toString())` relies on the
`toString()` format of `Namespace`, which is not guaranteed to be a parseable
identifier and can also produce identifiers inconsistent with the rest of the
codebase (leading to cache misses and extra REST calls). Construct the schema
identifier explicitly from the namespace levels (metalake/catalog/schema)
rather than round-tripping through `toString()`.
##########
clients/filesystem-hadoop3/src/main/java/org/apache/gravitino/filesystem/hadoop/DefaultGVFSOperations.java:
##########
@@ -124,32 +129,31 @@ public boolean rename(Path srcGvfsPath, Path dstGvfsPath)
throws IOException {
srcIdentifier,
dstIdentifier);
- Path srcActualPath =
- getActualFilePath(srcGvfsPath, currentLocationName(),
FilesetDataOperation.RENAME);
+ Pair<FileSystem, Path> src =
+ resolvePath(srcGvfsPath, currentLocationName(),
FilesetDataOperation.RENAME);
+ // Both paths are in the same fileset, asserted above, so the source's
filesystem serves both.
Path dstActualPath =
getActualFilePath(dstGvfsPath, currentLocationName(),
FilesetDataOperation.RENAME);
- FileSystem actualFs = getActualFileSystem(srcGvfsPath,
currentLocationName());
- return actualFs.rename(srcActualPath, dstActualPath);
+ return src.getLeft().rename(src.getRight(), dstActualPath);
Review Comment:
In `rename`, `resolvePath(...)` loads the catalog once, but
`getActualFilePath(...)` will load the same catalog again for the destination
path. This leaves `rename` with the same duplicated catalog lookup pattern the
PR is trying to eliminate. Consider adding a protected overload/helper (similar
to the new `getFileset(..., FilesetCatalog)`/`getSchema(..., Catalog)`) that
can compute the actual path using an already-resolved `FilesetCatalog` (and
possibly `Fileset`) so both src/dst use a single catalog load.
##########
clients/filesystem-hadoop3/src/test/java/org/apache/gravitino/filesystem/hadoop/TestGvfsBase.java:
##########
@@ -1755,4 +1755,58 @@ private void buildMockResourceForCredential(String
filesetName, String filesetLo
private MockGVFSHook getHook(FileSystem gvfs) {
return (MockGVFSHook) ((GravitinoVirtualFileSystem) gvfs).getHook();
}
+
+ @Test
+ public void testOneCatalogLoadPerOperation() throws IOException {
+ Assumptions.assumeTrue(getClass() == TestGvfsBase.class);
+ String filesetName = "testOneCatalogLoadPerOperation";
+ Path managedFilesetPath =
+ FileSystemTestUtils.createFilesetPath(catalogName, schemaName,
filesetName, true);
+ Path localPath = FileSystemTestUtils.createLocalDirPrefix(catalogName,
schemaName, filesetName);
+ String catalogPath = "/api/metalakes/" + metalakeName + "/catalogs/" +
catalogName;
+ String locationPath =
+ String.format(
+ "/api/metalakes/%s/catalogs/%s/schemas/%s/filesets/%s/location",
+ metalakeName, catalogName, schemaName,
RESTUtils.encodeString(filesetName));
+
+ try (FileSystem gravitinoFileSystem =
managedFilesetPath.getFileSystem(conf);
+ FileSystem localFileSystem = localPath.getFileSystem(conf)) {
+ FileSystemTestUtils.mkdirs(localPath, localFileSystem);
+ mockFilesetDTO(
+ metalakeName,
+ catalogName,
+ schemaName,
+ filesetName,
+ Fileset.Type.MANAGED,
+ ImmutableMap.of("location1", localPath.toString()),
+ ImmutableMap.of(PROPERTY_DEFAULT_LOCATION_NAME, "location1"));
+ Map<String, String> queryParams = new HashMap<>();
+ queryParams.put("sub_path", "/test.txt");
+ buildMockResource(
+ Method.GET,
+ locationPath,
+ queryParams,
+ null,
+ new FileLocationResponse(localPath + "/test.txt"),
+ SC_OK);
+ buildMockResourceForCredential(filesetName, localPath + "/test.txt");
+
+ // Warm the filesystem cache first: the very first operation
additionally constructs a
+ // FileSystem, which is what the deferred property map is for. Steady
state is the interesting
+ // number -- it is the one a query engine pays once per file.
+ Path filePath = new Path(managedFilesetPath + "/test.txt");
+ FileSystemTestUtils.create(filePath, gravitinoFileSystem);
+
+ HttpRequest catalogRequest = HttpRequest.request(catalogPath);
+ int before =
mockServer().retrieveRecordedRequests(catalogRequest).length;
+ FileSystemTestUtils.create(filePath, gravitinoFileSystem);
+ int after = mockServer().retrieveRecordedRequests(catalogRequest).length;
+
+ // Before this was collapsed, a single create resolved the catalog four
separate times.
+ assertEquals(
+ 1, after - before, "one filesystem operation must load the catalog
exactly once");
Review Comment:
This assertion is likely overly strict for the stated goal (collapsing
duplicates). If a future optimization removes the catalog call entirely on
steady-state cache hits, `after - before` could become `0` (an improvement) and
this test would fail. If the intent is ‘no more than once’, assert `<= 1`; if
the intent is ‘exactly once’, make the test explicitly configure/disable
metadata caching such that one catalog load is required, so the expectation is
stable.
--
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]
