Copilot commented on code in PR #9569:
URL: https://github.com/apache/gravitino/pull/9569#discussion_r2650322972
##########
catalogs/catalog-fileset/src/main/java/org/apache/gravitino/catalog/fileset/FilesetCatalogPropertiesMetadata.java:
##########
@@ -66,9 +71,12 @@ public class FilesetCatalogPropertiesMetadata extends
BaseCatalogPropertiesMetad
/** The value to indicate the cache value is not set. */
public static final long CACHE_VALUE_NOT_SET = -1;
- static final String FILESYSTEM_CONNECTION_TIMEOUT_SECONDS =
"filesystem-conn-timeout-secs";
+ public static final String FILESYSTEM_CONNECTION_TIMEOUT_SECONDS =
"filesystem-conn-timeout-secs";
static final int DEFAULT_GET_FILESYSTEM_TIMEOUT_SECONDS = 6;
+ /** The prefix for user-defined location configs: {@code
fs.path.config.<anme>=<path>}. */
Review Comment:
Typo in the Javadoc comment: 'anme' should be 'name'. The documentation says
'{@code fs.path.config.<anme>=<path>}' but it should be '{@code
fs.path.config.<name>=<path>}'.
```suggestion
/** The prefix for user-defined location configs: {@code
fs.path.config.<name>=<path>}. */
```
##########
catalogs/hadoop-common/src/main/java/org/apache/gravitino/catalog/hadoop/fs/HDFSFileSystemProvider.java:
##########
@@ -57,6 +62,21 @@ public String name() {
return BUILTIN_HDFS_FS_PROVIDER;
}
+ @Override
+ public String getFullAuthority(Path path, Map<String, String> conf) {
+ String authority = path.toUri().getAuthority();
+ String principal = conf.get(PRINCIPAL_KEY);
+ if (StringUtils.isNotBlank(principal)) {
+ principal = principal.replaceAll("@.*$", ""); // Remove realm if exists
+ authority = String.format("%s@%s", principal, authority);
+ }
+ String impersonationEnabled = conf.get(IMPERSONATION_ENABLE_KEY);
+ if (conf.containsKey(IMPERSONATION_ENABLE_KEY)) {
+ authority = String.format("%s?impersonation_enabled=%s)", authority,
impersonationEnabled);
Review Comment:
There's a missing opening parenthesis in the authority string formatting.
The closing parenthesis on line 75 should be removed, or an opening parenthesis
should be added. This will cause a malformed authority string.
```suggestion
authority = String.format("%s?impersonation_enabled=%s", authority,
impersonationEnabled);
```
##########
catalogs/catalog-fileset/src/test/java/org/apache/gravitino/catalog/fileset/integration/test/HadoopUserAuthenticationIT.java:
##########
@@ -552,7 +564,8 @@ void createFilesetWithKerberos() {
Assertions.assertDoesNotThrow(
() ->
catalog.asFilesetCatalog().dropFileset(NameIdentifier.of(SCHEMA_NAME,
fileset2)));
- Assertions.assertDoesNotThrow(() ->
catalog.asSchemas().dropSchema(SCHEMA_NAME, true));
+ Assertions.assertThrows(
+ Exception.class, () -> catalog.asSchemas().dropSchema(SCHEMA_NAME,
true));
Review Comment:
The test expectation was changed from assertDoesNotThrow to assertThrows for
dropping a schema. This appears to be an intentional behavior change to verify
permission handling, but there's no explanation for why this test expectation
was changed. If this is a bug fix, it should be documented in the test or PR
description. If this is an intentional behavior change for the new
implementation, it should be clearly documented.
##########
catalogs/catalog-fileset/src/main/java/org/apache/gravitino/catalog/fileset/FilesetCatalogOperations.java:
##########
@@ -166,19 +173,14 @@ static class FileSystemCacheKey {
// authority are both null
@Nullable private final String scheme;
@Nullable private final String authority;
- private final Map<String, String> conf;
+ private String locationName;
Review Comment:
The locationName field should be marked as @Nullable since it can be null
when passed to the constructor. The equals() method already handles null
comparison correctly with Objects.equals(), but the field declaration should
reflect that it's nullable for clarity.
```suggestion
@Nullable private String locationName;
```
##########
catalogs/catalog-common/src/main/java/org/apache/gravitino/utils/FilesetUtil.java:
##########
@@ -0,0 +1,138 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.gravitino.utils;
+
+import com.google.common.base.Preconditions;
+import java.net.URI;
+import java.util.HashMap;
+import java.util.Map;
+import org.apache.commons.lang3.StringUtils;
+
+/** Utility class for fileset-related operations. */
+public class FilesetUtil {
+
+ private FilesetUtil() {}
+
+ /**
+ * Get user-defined configurations for a given path based on the path's base
location
+ * (scheme://authority).
+ *
+ * <p>The logic:
+ *
+ * <ol>
+ * <li>Extract baseLocation (scheme://authority) from the given path
+ * <li>Find config entries like "fs.path.config.<name> =
<base_location>" where the
+ * base_location matches the extracted baseLocation
+ * <li>Extract the name from the matching entry
+ * <li>Then find all config entries with prefix
"fs.path.config.<name>." and extract
+ * properties
+ * </ol>
+ *
+ * <p>Example:
+ *
+ * <pre>
+ * fs.path.config.cluster1 = s3://bucket1
+ * fs.path.config.cluster1.aws-access-key = XXX1
+ * fs.path.config.cluster1.aws-secret-key = XXX2
+ * If path is "s3://bucket1/path/fileset1", then baseLocation is
"s3://bucket1",
+ * cluster1 matches and we extract:
+ * - aws-access-key = XXX1
+ * - aws-secret-key = XXX2
+ * </pre>
+ *
+ * @param path the path to extract configurations for
+ * @param conf the configuration map containing path-based configurations
+ * @param configPrefix the prefix for path-based configurations (e.g.,
"fs.path.config.")
+ * @return a map of user-defined configurations for the given path
+ */
+ public static Map<String, String> getUserDefinedFileSystemConfigs(
+ URI path, Map<String, String> conf, String configPrefix) {
+ Preconditions.checkArgument(path != null, "Path should not be null");
+ Preconditions.checkArgument(conf != null, "Configuration map should not be
null");
+ Preconditions.checkArgument(
+ StringUtils.isNotBlank(configPrefix), "Config prefix should not be
blank");
+
+ Map<String, String> properties = new HashMap<>();
+ String baseLocation = getBaseLocation(path);
+ String locationName = null;
+
+ // First pass: find the location name by matching baseLocation
+ // Look for entries like "fs.path.config.<name> = <base_location>"
+ // The key format should be exactly "fs.path.config.<name>" (no dot after
name)
+ for (Map.Entry<String, String> entry : conf.entrySet()) {
+ String key = entry.getKey();
+ String value = entry.getValue();
+
+ // Check if this is a location definition: "fs.path.config.<name> =
<base_location>"
+ // The key format should be exactly "fs.path.config.<name>" (no dot
after name)
+ if (key.startsWith(configPrefix)) {
+ String suffix = key.substring(configPrefix.length());
+ // Check if this is a location definition (no dot after the name)
+ // Format: "fs.path.config.<name>" (not
"fs.path.config.<name>.<property>")
+ if (!suffix.contains(".")
+ && StringUtils.isNotBlank(suffix)
+ && StringUtils.isNotBlank(value)) {
+ // This is a location definition: "fs.path.config.<name>"
+ // Extract baseLocation from the value and compare with the path's
baseLocation
+ try {
+ URI valueUri = new URI(value);
+ String valueBaseLocation = getBaseLocation(valueUri);
+ if (baseLocation.equals(valueBaseLocation)) {
+ locationName = suffix;
+ break;
+ }
+ } catch (Exception e) {
+ // Skip invalid URI values
+ continue;
+ }
+ }
+ }
+ }
+
+ // Second pass: extract all properties for the matched location name
+ if (locationName != null) {
+ String propertyPrefix = configPrefix + locationName + ".";
+ for (Map.Entry<String, String> entry : conf.entrySet()) {
+ String key = entry.getKey();
+ // Check if this key is a property for the matched location
+ // e.g., "fs.path.config.cluster1.aws-ak" matches prefix
"fs.path.config.cluster1."
+ if (key.startsWith(propertyPrefix)) {
+ // Extract the property name after the location prefix
+ // e.g., "fs.path.config.cluster1.aws-ak" -> "aws-ak"
+ String propertyName = key.substring(propertyPrefix.length());
+ if (!propertyName.isEmpty()) {
+ properties.put(propertyName, entry.getValue());
+ }
+ }
+ }
+ }
+
+ return properties;
+ }
+
+ /**
+ * Extract base location (scheme://authority) from a URI.
+ *
+ * @param uri the URI to extract base location from
+ * @return the base location in format "scheme://authority"
+ */
+ private static String getBaseLocation(URI uri) {
+ return uri.getScheme() + "://" + uri.getAuthority();
Review Comment:
The getBaseLocation method doesn't handle the case where uri.getAuthority()
returns null. For file:// URIs or other URIs without an authority component,
this will result in a string like "file://null". Consider adding a null check
and handling this case appropriately.
```suggestion
String scheme = uri.getScheme();
String authority = uri.getAuthority();
// For URIs without an authority component (e.g., file:/path or
file:///path),
// getAuthority() returns null. In that case, we return "scheme://"
// instead of "scheme://null" so that configuration values like "file://"
// can match correctly.
if (authority == null) {
return scheme + "://";
}
return scheme + "://" + authority;
```
--
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]
