Copilot commented on code in PR #13891:
URL: https://github.com/apache/gravitino/pull/13891#discussion_r4290783741
##########
clients/filesystem-hadoop3/src/main/java/org/apache/gravitino/filesystem/hadoop/BaseGVFSOperations.java:
##########
@@ -969,18 +982,27 @@ private Cache<FileSystemCacheKey, FileSystem>
newFileSystemCache(Configuration c
@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()));
+ return getAllProperties(filesetIdent, null);
+ }
+
+ @VisibleForTesting
+ Map<String, String> getAllProperties(NameIdentifier filesetIdent, @Nullable
Fileset fileset) {
+ NameIdentifier catalogIdent =
+ NameIdentifier.of(filesetIdent.namespace().level(0),
filesetIdent.namespace().level(1));
+ NameIdentifier schemaIdent =
+ NameIdentifier.of(
+ filesetIdent.namespace().level(0),
+ filesetIdent.namespace().level(1),
+ filesetIdent.namespace().level(2));
+ Catalog catalog = getCatalog(catalogIdent);
+ Schema schema = getSchema(schemaIdent);
+ Fileset resolvedFileset = fileset != null ? fileset :
getFileset(filesetIdent);
Review Comment:
With metadata caching disabled, this flow can trigger multiple
`loadCatalog(...)` calls per `getAllProperties` invocation: `getAllProperties`
loads the catalog via `getCatalog(...)`, and then `getSchema(...)` (and
potentially `getFileset(...)`) can load the catalog again in their uncached
code paths (since they delegate to `getCatalog(...)`). This is a concrete
regression vs. the previous implementation which loaded the catalog once and
reused it for schema/fileset resolution. Consider avoiding the extra RPCs by
reusing the already-resolved `catalog` when the metadata cache is disabled
(e.g., in `getAllProperties`, only call the cache for schema/fileset when
present; otherwise resolve schema/fileset from the `catalog` instance directly,
or add overloads that accept a preloaded `Catalog`/`FilesetCatalog`).
--
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]
