pithecuse527 commented on code in PR #12857:
URL: https://github.com/apache/gravitino/pull/12857#discussion_r3960641960
##########
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:
This can be addressed by introducing a new helper method
(BaseGVFSOperations::resolveRenamePaths) to handle this
##########
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:
This can be addressed by introducing a new helper method
(BaseGVFSOperations::resolveRenamePaths) to handle this
##########
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:
This can be addressed by introducing a new helper method
(BaseGVFSOperations::resolveRenamePaths) to handle this
--
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]
