roryqi commented on code in PR #10895:
URL: https://github.com/apache/gravitino/pull/10895#discussion_r3246166150
##########
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:
Could u avoid null value here?
##########
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:
You can reuse `Boolean.TRUE.equals(request.getVendCredentials());` directly.
It can handle null value.
##########
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:
Use null value is not good design. For many internal code, we don't handle
null value.
Usually we use optional instead.
##########
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:
Could u put `CatalogCredentialManager` into Catalog? Could u create a
catalogCredentialWrapper for this?
##########
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:
Why do use `catalogName` instead of `catalog.name()`? It is duplicated.
--
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]
