sunyuhan1998 commented on code in PR #10895:
URL: https://github.com/apache/gravitino/pull/10895#discussion_r3212901206
##########
common/src/main/java/org/apache/gravitino/credential/CredentialPropertyUtils.java:
##########
@@ -51,6 +51,24 @@ public class CredentialPropertyUtils {
private static final String GCS_OAUTH_2_TOKEN_EXPIRES_AT =
"gcs.oauth2.token-expires-at";
+ // Lance storage_options key constants
+ @VisibleForTesting static final String LANCE_S3_ACCESS_KEY_ID =
"aws_access_key_id";
Review Comment:
These constants and the `lanceCredentialPropertyMap` / `toLanceProperties()`
are indeed Lance-specific, but `CredentialPropertyUtils` follows a centralized
pattern where all table format credential transformations live in one class.
The Iceberg equivalents (`ICEBERG_S3_ACCESS_KEY_ID`, `toIcebergProperties()`,
etc.) are in the same class. Keeping them together seems consistent with the
existing pattern. Would it be acceptable to keep it as is, or do you think
moving them to the lance module would be better?
##########
lance/lance-rest-server/src/main/java/org/apache/gravitino/lance/service/rest/LanceTableOperations.java:
##########
@@ -90,16 +96,37 @@ public Response describeTable(
DescribeTableRequest request) {
try {
validateDescribeTableRequest(request);
+ boolean vendCredentials =
+ request.getVendCredentials() == null ||
Boolean.TRUE.equals(request.getVendCredentials());
Review Comment:
No, credentials are only injected when the client explicitly sets
`vendCredentials = true` in the request. When `vendCredentials` is not set or
is `false`, `credentialPrivilege` remains `null`, and the code falls into the
`else` branch which returns only the original storage options without any
credentials. See the `if (credentialPrivilege != null)` check in
`GravitinoLanceTableOperations.describeTable()`.
--
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]
