sunyuhan1998 commented on code in PR #10895:
URL: https://github.com/apache/gravitino/pull/10895#discussion_r3255978935
##########
lance/lance-common/src/main/java/org/apache/gravitino/lance/common/ops/gravitino/GravitinoLanceTableOperations.java:
##########
@@ -131,10 +159,61 @@ public DescribeTableResponse describeTable(
Optional.ofNullable(table.properties().get(LANCE_TABLE_VERSION))
.map(Long::valueOf)
.orElse(null));
-
response.setStorageOptions(LancePropertiesUtils.getLanceStorageOptions(table.properties()));
+
+ if (credentialPrivilege != null) {
+ response.setStorageOptions(
+ buildVendedStorageOptions(catalogName, catalog, table,
credentialPrivilege));
+ } else {
+
response.setStorageOptions(LancePropertiesUtils.getLanceStorageOptions(table.properties()));
+ }
+
return response;
}
+ private Map<String, String> buildVendedStorageOptions(
+ String catalogName, Catalog catalog, Table table, CredentialPrivilege
credentialPrivilege) {
Review Comment:
Thanks for the feedback. Let me address both comments together since they
relate to the same architectural question.
**Regarding the credential generation approach:**
We investigated how the Iceberg REST server handles credential vending. In
[`CatalogWrapperForREST`](https://github.com/apache/gravitino/blob/main/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/CatalogWrapperForREST.java#L129),
the `CatalogCredentialManager` is created locally and credentials are
generated in-process via Java SPI-loaded `CredentialProvider`s — there is no
HTTP call to the Gravitino server for credentials. Our current implementation
follows the same pattern.
**Regarding the backend-agnostic concern (agree with @FANNG1):**
We agree that credential vending should not be tied to the Gravitino-backed
implementation. We plan to refactor as follows:
1. **Create `LanceCredentialVendingService`** in `lance-common` — a
Gravitino-agnostic service that handles credential generation. It takes catalog
name, catalog properties, table location, and privilege as input, and returns
credential properties for Lance `storage_options`.
2. **Simplify `GravitinoLanceTableOperations`** — it will only handle table
operations (proxy to Gravitino server) and return the base
`DescribeTableResponse` without credentials. All credential-related code will
be removed.
3. **Credential injection at the wrapper/REST layer** — similar to how
Iceberg's `CatalogWrapperForREST` injects credentials at the wrapper level (not
inside individual table operations), we will orchestrate credential injection
at the namespace wrapper or REST layer.
This way, any Lance namespace backend can use the
`LanceCredentialVendingService` by simply providing catalog properties and
table location, without depending on the Gravitino backend.
@roryqi @FANNG1 Does this approach make sense to you? Any concerns or
suggestions?
##########
lance/lance-common/src/main/java/org/apache/gravitino/lance/common/ops/gravitino/GravitinoLanceTableOperations.java:
##########
@@ -131,10 +159,61 @@ public DescribeTableResponse describeTable(
Optional.ofNullable(table.properties().get(LANCE_TABLE_VERSION))
.map(Long::valueOf)
.orElse(null));
-
response.setStorageOptions(LancePropertiesUtils.getLanceStorageOptions(table.properties()));
+
+ if (credentialPrivilege != null) {
+ response.setStorageOptions(
+ buildVendedStorageOptions(catalogName, catalog, table,
credentialPrivilege));
+ } else {
+
response.setStorageOptions(LancePropertiesUtils.getLanceStorageOptions(table.properties()));
+ }
+
return response;
}
+ private Map<String, String> buildVendedStorageOptions(
+ String catalogName, Catalog catalog, Table table, CredentialPrivilege
credentialPrivilege) {
Review Comment:
(See reply above on @roryqi 's comment for the full response)
We agree with the backend-agnostic approach. We plan to extract credential
vending into a `LanceCredentialVendingService` in `lance-common` that is not
tied to any specific Lance namespace backend. See the detailed plan above. Let
us know if you have any concerns or suggestions.
--
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]
