collado-mike commented on code in PR #1164:
URL: https://github.com/apache/polaris/pull/1164#discussion_r2038423045
##########
quarkus/service/src/test/java/org/apache/polaris/service/quarkus/auth/JWTSymmetricKeyGeneratorTest.java:
##########
@@ -24,6 +24,7 @@
import com.auth0.jwt.JWTVerifier;
import com.auth0.jwt.algorithms.Algorithm;
import com.auth0.jwt.interfaces.DecodedJWT;
+import java.net.URI;
Review Comment:
unnecessary import?
##########
service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogAdapter.java:
##########
@@ -378,6 +378,7 @@ public Response loadTable(
Namespace ns = decodeNamespace(namespace);
TableIdentifier tableIdentifier = TableIdentifier.of(ns,
RESTUtil.decodeString(table));
+
Review Comment:
I don't know why these extra blank lines are added
##########
service/common/src/testFixtures/java/org/apache/polaris/service/TestServices.java:
##########
@@ -21,6 +21,7 @@
import com.google.auth.oauth2.AccessToken;
import com.google.auth.oauth2.GoogleCredentials;
import jakarta.ws.rs.core.SecurityContext;
+import java.net.URI;
Review Comment:
same
##########
service/common/src/test/java/org/apache/polaris/service/catalog/io/FileIOFactoryTest.java:
##########
@@ -25,6 +25,7 @@
import com.google.common.collect.ImmutableMap;
import jakarta.annotation.Nonnull;
import java.lang.reflect.Method;
+import java.net.URI;
Review Comment:
unnecessary import?
##########
service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java:
##########
@@ -856,6 +857,15 @@ public Map<String, String> getCredentialConfig(
storageInfo.get());
}
+ @Override
+ public Map<String, String> getVendedCredentialConfig(TableIdentifier
tableIdentifier, String decodedCredentialsPath) {
Review Comment:
Why is this a whole new method rather than just putting this directly in the
`IcebergCatalogHandler`. You're just putting values into a map, so...
##########
service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogAdapter.java:
##########
@@ -392,15 +393,20 @@ public Response loadTable(
catalog -> {
LoadTableResponse response;
+
if (delegationModes.isEmpty()) {
response =
catalog
.loadTableIfStale(tableIdentifier, ifNoneMatch, snapshots)
.orElseThrow(() -> new
WebApplicationException(Response.Status.NOT_MODIFIED));
} else {
+ String credentialsEndpoint =
+ String.format(
+ "/v1/%s/namespaces/%s/tables/%s/credentials",
+ prefix, tableIdentifier.namespace().toString(),
tableIdentifier.name());
Review Comment:
This needs to be configurable. E.g., some hosts may prefix the URL with
`/polaris` or with `/your_account_name` or any number of prefixes
--
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: issues-unsubscr...@polaris.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
