bharos commented on code in PR #10586:
URL: https://github.com/apache/gravitino/pull/10586#discussion_r3042177971
##########
iceberg/iceberg-common/src/main/java/org/apache/gravitino/iceberg/common/ops/IcebergCatalogWrapper.java:
##########
@@ -414,4 +440,114 @@ private TableMetadataCache
loadTableMetadataCache(IcebergConfig config, Catalog
expireMinutes);
return cache;
}
+
+ private LoadTableResponse loadTableInternal(TableIdentifier ident) {
+ if (!(catalog instanceof RESTCatalog)) {
+ return CatalogHandlers.loadTable(catalog, ident);
+ }
+
+ Table table = catalog.loadTable(ident);
+
+ if (table instanceof BaseTable) {
+ Map<String, String> properties = retrieveFileIOProperties(table.io());
+ return LoadTableResponse.builder()
+ .withTableMetadata(((BaseTable) table).operations().current())
+ .addAllConfig(
+ MapUtils.getFilteredMap(
+ properties, key ->
catalogPropertiesToClientKeys.contains(key)))
+ // Keep only credential fields from FileIO properties before
returning them to the client.
+
.addAllConfig(CredentialPropertyUtils.filterCredentialProperties(properties))
+ .build();
+ } else if (table instanceof BaseMetadataTable) {
+ // metadata tables are loaded on the client side, return
NoSuchTableException for now
+ throw new NoSuchTableException("Table does not exist: %s",
ident.toString());
+ }
+
+ throw new IllegalStateException("Cannot wrap catalog that does not produce
BaseTable");
+ }
+
+ private LoadTableResponse createTableInternal(Namespace namespace,
CreateTableRequest request) {
+ if (!(catalog instanceof RESTCatalog)) {
+ return CatalogHandlers.createTable(catalog, namespace, request);
+ }
+
+ request.validate();
+
+ TableIdentifier ident = TableIdentifier.of(namespace, request.name());
+ Table table =
+ catalog
+ .buildTable(ident, request.schema())
+ .withLocation(request.location())
+ .withPartitionSpec(request.spec())
+ .withSortOrder(request.writeOrder())
+ .withProperties(request.properties())
+ .create();
+
+ if (table instanceof BaseTable) {
+ Map<String, String> properties = retrieveFileIOProperties(table.io());
+ return LoadTableResponse.builder()
+ .withTableMetadata(((BaseTable) table).operations().current())
+ .addAllConfig(
+ MapUtils.getFilteredMap(
+ properties, key ->
catalogPropertiesToClientKeys.contains(key)))
+ // Keep only credential fields from FileIO properties before
returning them to the client.
+
.addAllConfig(CredentialPropertyUtils.filterCredentialProperties(properties))
+ .build();
+ }
+
+ throw new IllegalStateException("Cannot wrap catalog that does not produce
BaseTable");
+ }
+
+ public LoadTableResponse stageTableCreateInternal(
Review Comment:
This doesn't need to be public ?
##########
iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/CatalogWrapperForREST.java:
##########
@@ -186,6 +181,64 @@ public Map<String, String> getCatalogConfigToClient() {
return catalogConfigToClients;
}
+ /**
+ * Builds properties exposed to Iceberg clients via the IRC {@code
/v1/config} defaults.
+ *
+ * <p>For {@link RESTCatalog}, uses {@link RESTCatalog#properties()} so
defaults reflect the
+ * remote catalog's config response merged with client properties (after
REST handshake), not only
+ * static Gravitino catalog configuration.
+ */
+ @VisibleForTesting
+ static Map<String, String> buildCatalogConfigToClients(IcebergConfig config,
Catalog catalog) {
+ Map<String, String> sourceProps;
+ if (catalog instanceof RESTCatalog) {
+ Map<String, String> merged = ((RESTCatalog) catalog).properties();
+ sourceProps = merged != null ? new HashMap<>(merged) : new HashMap<>();
+ } else {
+ sourceProps = new HashMap<>(config.getIcebergCatalogProperties());
+ }
+
+ Map<String, String> filtered =
+ MapUtils.getFilteredMap(sourceProps, key ->
catalogPropertiesToClientKeys.contains(key));
+ filtered = new HashMap<>(filtered);
+ validateAndNormalizeDataAccessProperty(filtered);
+
+ return Collections.unmodifiableMap(filtered);
+ }
+
+ @VisibleForTesting
+ static void validateAndNormalizeDataAccessProperty(Map<String, String>
properties) {
+ String dataAccess =
properties.get(IcebergConstants.ICEBERG_ACCESS_DELEGATION);
+ if (StringUtils.isBlank(dataAccess)) {
+ return;
+ }
+
+ String normalizedDataAccess = dataAccess.toLowerCase(Locale.ROOT);
+ if (!DATA_ACCESS_VENDED_CREDENTIALS.equals(normalizedDataAccess)
+ && !DATA_ACCESS_REMOTE_SIGNING.equals(normalizedDataAccess)) {
+ throw new IllegalArgumentException(
+ "Invalid catalog property '"
+ + IcebergConstants.DATA_ACCESS
+ + "': "
+ + dataAccess
+ + ", supported values are ["
+ + DATA_ACCESS_VENDED_CREDENTIALS
+ + ","
+ + DATA_ACCESS_REMOTE_SIGNING
+ + "]");
+ }
+
+ properties.put(IcebergConstants.DATA_ACCESS, normalizedDataAccess);
+ }
+
+ private static void putIfValuePresent(
Review Comment:
Is this unused ?
##########
iceberg/iceberg-common/src/main/java/org/apache/gravitino/iceberg/common/ops/IcebergCatalogWrapper.java:
##########
@@ -414,4 +440,114 @@ private TableMetadataCache
loadTableMetadataCache(IcebergConfig config, Catalog
expireMinutes);
return cache;
}
+
+ private LoadTableResponse loadTableInternal(TableIdentifier ident) {
Review Comment:
What if we make these 3 methods `protected` in the base class with the
default CatalogHandlers behavior,and override them in CatalogWrapperForREST:
```
// Base class — clean defaults, no REST knowledge
protected LoadTableResponse loadTableInternal(TableIdentifier ident) {
return CatalogHandlers.loadTable(catalog, ident);
}
protected LoadTableResponse createTableInternal(Namespace namespace,
CreateTableRequest request) {
return CatalogHandlers.createTable(catalog, namespace, request);
}
protected LoadTableResponse stageTableCreateInternal(Namespace namespace,
CreateTableRequest request) {
return CatalogHandlers.stageTableCreate(catalog, namespace, request);
}
```
and
```
// CatalogWrapperForREST — override with REST logic
@Override
protected LoadTableResponse loadTableInternal(TableIdentifier ident) {
Table table = catalog.loadTable(ident);
// ... BaseTable handling, credential filtering, etc.
}
```
This also lets catalogPropertiesToClientKeys and retrieveFileIOProperties()
move back to the subclass, and removes all REST imports (RESTCatalog,
BaseTable, BaseMetadataTable, InMemoryFileIO, etc.) from the base class.
##########
docs/iceberg-rest-service.md:
##########
@@ -157,6 +158,11 @@ gravitino.iceberg-rest.header.X-Iceberg-Access-Delegation
= vended-credentials
IRC1 must also configure S3 configurations if the client side requests
credential vending.
+`data-access` is returned in `/v1/config` defaults for REST clients:
+
+- `vended-credentials`: clients should request credential vending
(`X-Iceberg-Access-Delegation: vended-credentials`).
+- `remote-signing`: Gravitino doesn't support now.
Review Comment:
nit : "Gravitino does not support this mode yet"
##########
iceberg/iceberg-common/src/main/java/org/apache/gravitino/iceberg/common/ops/IcebergCatalogWrapper.java:
##########
@@ -239,7 +265,7 @@ public LoadTableResponse updateTable(
metadataCache.invalidate(tableIdentifier);
LoadTableResponse loadTableResponse =
CatalogHandlers.updateTable(catalog, tableIdentifier,
updateTableRequest);
- if (loadTableResponse != null) {
+ if (loadTableResponse != null && !(catalog instanceof RESTCatalog)) {
Review Comment:
The metadata cache skip (!(catalog instanceof RESTCatalog)) can similarly
become a protected boolean shouldCacheMetadata() that the subclass overrides to
return false.
##########
docs/lakehouse-iceberg-catalog.md:
##########
@@ -71,6 +71,15 @@ If you are using multiple JDBC catalog backends, setting
`jdbc-initialize` to tr
For the REST catalog backend, `warehouse` identifies the catalog in the
Iceberg REST spec. In the Gravitino Iceberg REST server, `warehouse` maps to
the catalog name. An empty value means the default catalog.
+`data-access` controls how the Iceberg REST client accesses table data when
using a REST backend:
+
+| Property name | Description
| Default value |
Required | Since Version |
+|----------------|-------------------------------------------------------------------------------------------------------------------------|---------------|----------|---------------|
+| `data-access` | Data access mode for REST catalog backend. Supported values
are `vended-credentials` and `remote-signing`. | (none) |
No | 1.3.0 |
+
+- `vended-credentials`: request credential vending from the Iceberg REST
server.
+- `remote-signing`: Gravitino doesn't support now.
Review Comment:
nit : "Gravitino does not support this mode yet"
--
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]
