sunyuhan1998 commented on code in PR #10895:
URL: https://github.com/apache/gravitino/pull/10895#discussion_r3246490769
##########
lance/lance-common/src/main/java/org/apache/gravitino/lance/common/ops/gravitino/GravitinoLanceTableOperations.java:
##########
@@ -98,9 +112,23 @@ public
GravitinoLanceTableOperations(GravitinoLanceNamespaceWrapper namespaceWra
this.namespaceWrapper = namespaceWrapper;
}
+ public void close() {
+ for (Map.Entry<String, CatalogCredentialManager> entry :
credentialManagers.entrySet()) {
+ try {
+ entry.getValue().close();
+ } catch (Exception e) {
+ LOG.warn("Error closing credential manager for catalog {}",
entry.getKey(), e);
+ }
+ }
+ credentialManagers.clear();
+ }
+
@Override
public DescribeTableResponse describeTable(
- String tableId, String delimiter, Optional<Long> version) {
+ String tableId,
+ String delimiter,
+ Optional<Long> version,
+ @Nullable CredentialPrivilege credentialPrivilege) {
Review Comment:
Done. We have changed the parameter to `Optional<CredentialPrivilege>` in
the `LanceTableOperations` interface and updated all callers/implementations.
##########
lance/lance-rest-server/src/main/java/org/apache/gravitino/lance/service/rest/LanceTableOperations.java:
##########
@@ -90,16 +96,38 @@ public Response describeTable(
DescribeTableRequest request) {
try {
validateDescribeTableRequest(request);
+ boolean vendCredentials =
+ request.getVendCredentials() == null ||
Boolean.TRUE.equals(request.getVendCredentials());
Review Comment:
The extra null check (`request.getVendCredentials() == null ||`) is
intentional — it defaults `vendCredentials` to `true` when not specified,
following the [Lance official reference
implementation](https://github.com/lancedb/lance-namespace/blob/56a32a24c12fd73e38b3c4fa9f39bf4fd83b7730/src/provider.rs#L57)
where `unwrap_or(true)` is used. That said, we are open to changing the
default to `false` if you think that is more appropriate. What do you think?
##########
lance/lance-rest-server/src/main/java/org/apache/gravitino/lance/service/rest/LanceTableOperations.java:
##########
@@ -90,16 +96,38 @@ public Response describeTable(
DescribeTableRequest request) {
try {
validateDescribeTableRequest(request);
+ boolean vendCredentials =
+ request.getVendCredentials() == null ||
Boolean.TRUE.equals(request.getVendCredentials());
+ CredentialPrivilege privilege =
+ vendCredentials ? getCredentialPrivilege(tableId, delimiter) : null;
Review Comment:
Done in the latest commit. We have refactored to use
`Optional<CredentialPrivilege>` throughout the call chain — from the REST layer
through the interface down to the implementation.
--
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]
