Copilot commented on code in PR #9507:
URL: https://github.com/apache/gravitino/pull/9507#discussion_r2638880751
##########
clients/filesystem-hadoop3/src/main/java/org/apache/gravitino/filesystem/hadoop/GravitinoVirtualFileSystemUtils.java:
##########
@@ -324,6 +326,13 @@ public static String getSubPathFromGvfsPath(NameIdentifier
identifier, String gv
return gvfsPath.substring(prefix.length());
}
+ static void setCallerContextForGetCredentials(String locationName) {
Review Comment:
The setCallerContextForGetCredentials method should validate that
locationName is not null before putting it in the context map. If locationName
is null, it may lead to unexpected behavior when the CallerContext is used on
the server side. Consider adding a Preconditions check or handling null
explicitly by either rejecting it or setting a default value.
```suggestion
static void setCallerContextForGetCredentials(String locationName) {
Preconditions.checkNotNull(locationName, "The location name cannot be
null.");
```
##########
clients/filesystem-hadoop3/src/main/java/org/apache/gravitino/filesystem/hadoop/GravitinoVirtualFileSystemUtils.java:
##########
@@ -324,6 +326,13 @@ public static String getSubPathFromGvfsPath(NameIdentifier
identifier, String gv
return gvfsPath.substring(prefix.length());
}
+ static void setCallerContextForGetCredentials(String locationName) {
+ Map<String, String> contextMap = Maps.newHashMap();
+ contextMap.put(CredentialConstants.HTTP_HEADER_CURRENT_LOCATION_NAME,
locationName);
+ CallerContext callerContext =
CallerContext.builder().withContext(contextMap).build();
+ CallerContext.CallerContextHolder.set(callerContext);
+ }
Review Comment:
This new method is missing Javadoc documentation. Following the Apache
Gravitino coding guidelines, public APIs and utility methods should be
well-documented. The method should include:
- A description of what the method does (sets caller context for getting
credentials with location-specific information)
- A @param tag explaining the locationName parameter
- An explanation of the side effect (sets thread-local CallerContext that
must be cleaned up by caller)
##########
clients/filesystem-hadoop3/src/main/java/org/apache/gravitino/filesystem/hadoop/DefaultGravitinoFileSystemCredentialsProvider.java:
##########
@@ -51,10 +56,31 @@ public Credential[] getCredentials() {
// The format of name identifier is `metalake.catalog.schema.fileset`
String nameIdentifier = configuration.get(GVFS_NAME_IDENTIFIER);
String[] idents = nameIdentifier.split("\\.");
+
try (GravitinoClient client =
GravitinoVirtualFileSystemUtils.createClient(configuration)) {
FilesetCatalog filesetCatalog =
client.loadCatalog(idents[1]).asFilesetCatalog();
Fileset fileset =
filesetCatalog.loadFileset(NameIdentifier.of(idents[2], idents[3]));
+
+ String targetLocationName = getTargetLocation(fileset);
+
GravitinoVirtualFileSystemUtils.setCallerContextForGetCredentials(targetLocationName);
+
return fileset.supportsCredentials().getCredentials();
+ } finally {
+ CallerContext.CallerContextHolder.remove();
}
}
+
+ private String getTargetLocation(Fileset fileset) {
+ String currentLocationEnvVar =
+ configuration.get(
+
GravitinoVirtualFileSystemConfiguration.FS_GRAVITINO_CURRENT_LOCATION_NAME_ENV_VAR,
+ GravitinoVirtualFileSystemConfiguration
+ .FS_GRAVITINO_CURRENT_LOCATION_NAME_ENV_VAR_DEFAULT);
+ String locationName =
+
Optional.ofNullable(configuration.get(FS_GRAVITINO_CURRENT_LOCATION_NAME))
+ .orElse(System.getenv(currentLocationEnvVar));
+ return locationName == null
+ ? fileset.properties().get(PROPERTY_DEFAULT_LOCATION_NAME)
+ : locationName;
+ }
Review Comment:
The new logic in
DefaultGravitinoFileSystemCredentialsProvider.getTargetLocation for handling
multiple locations lacks test coverage. While there are tests for multiple
locations in GravitinoVirtualFileSystemIT.testCurrentLocationName, there's no
test verifying that credentials are correctly fetched for specific locations in
a multi-location fileset scenario. This is critical functionality that should
be tested to prevent regressions, especially given that this PR is specifically
fixing credential handling for filesets with multiple locations.
--
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]
