yuqi1129 commented on code in PR #12555:
URL: https://github.com/apache/gravitino/pull/12555#discussion_r3843679567
##########
trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/catalog/iceberg/IcebergCatalogPropertyConverter.java:
##########
@@ -152,6 +246,99 @@ private Map<String, String>
buildJDBCBackendProperties(Map<String, String> prope
return jdbcProperties;
}
+ private void warnOnReservedOverrides(GravitinoCatalog catalog, Map<String,
String> config) {
+ for (String reserved : RESERVED_REST_PROPERTIES) {
+ if (config.containsKey(reserved)) {
+ LOG.info(
+ "Property '{}' set on catalog '{}' is ignored; the connector
always derives it when "
+ + "routing through the Iceberg REST server.",
+ reserved,
+ catalog.getName());
+ }
+ }
+ }
+
+ /**
+ * Derives the Trino file system config from the catalog's warehouse
location. Vended credentials
+ * are only consumed by Trino's native file systems, so the native
implementation matching the
+ * warehouse scheme has to be enabled.
+ */
+ private Map<String, String> buildStorageProperties(Map<String, String>
properties) {
+ Map<String, String> storageProperties = new HashMap<>();
+ // Always available as the fallback; where a native file system is also
enabled below, that one
+ // takes precedence for its own scheme.
+ storageProperties.put(TRINO_FS_HADOOP_ENABLED, "true");
+
+ String warehouse = properties.get(IcebergConstants.WAREHOUSE);
Review Comment:
Native filesystem selection is based only on the catalog warehouse, but
credentials are vended per table location. A valid Hive catalog may have no
warehouse, or an HDFS warehouse with a table explicitly located at s3://... and
credential-providers=s3-token. In both cases this returns with only
fs.hadoop.enabled, so IRC can vend an S3 session credential that Trino still
cannot consume. Please derive native filesystem enablement from
credential-providers as well, or otherwise support per-table schemes, and cover
no-warehouse/different-table-location cases with a MinIO STS end-to-end test.
##########
trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/catalog/iceberg/IcebergConnectorAdapter.java:
##########
@@ -20,42 +20,131 @@
import static java.util.Collections.emptyList;
+import com.google.common.collect.ImmutableMap;
import io.trino.spi.session.PropertyMetadata;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
-import org.apache.gravitino.catalog.property.PropertyConverter;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.gravitino.catalog.lakehouse.iceberg.IcebergConstants;
import org.apache.gravitino.credential.Credential;
+import org.apache.gravitino.trino.connector.GravitinoConfig;
import org.apache.gravitino.trino.connector.catalog.CatalogConnectorAdapter;
import
org.apache.gravitino.trino.connector.catalog.CatalogConnectorMetadataAdapter;
import org.apache.gravitino.trino.connector.metadata.GravitinoCatalog;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
/**
* Transforming Apache Iceberg connector configuration and components into
Apache Gravitino
* connector.
*/
public class IcebergConnectorAdapter implements CatalogConnectorAdapter {
+ private static final Logger LOG =
LoggerFactory.getLogger(IcebergConnectorAdapter.class);
+
private static final String CONNECTOR_ICEBERG = "iceberg";
+ private static final String REST_CATALOG_BACKEND = "rest";
+ private static final String ICEBERG_PROVIDER = "lakehouse-iceberg";
+
+ /**
+ * Synthetic catalog property carrying the Iceberg REST server endpoint the
coordinator discovered
+ * for this catalog's metalake. {@link GravitinoConfig}'s own
discovered-endpoint map is populated
+ * only on the coordinator (the periodic discovery poll never runs on a
worker), so it cannot be
+ * read directly when building a catalog's internal connector config: every
node needs the same
+ * routing decision for the same catalog. Embedding the resolved endpoint
into the catalog itself,
+ * at registration time, means it travels to every node through the {@code
CREATE CATALOG}
+ * statement Trino replicates cluster-wide, the same way any other catalog
property does.
+ */
+ static final String DISCOVERED_ICEBERG_REST_URI_PROPERTY =
"__gravitino.iceberg.rest-uri";
+
private final IcebergPropertyMeta propertyMetadata;
- private final PropertyConverter catalogConverter;
+ private final IcebergCatalogPropertyConverter catalogConverter;
+ private final GravitinoConfig config;
/**
* Constructs a new IcebergConnectorAdapter. Initializes the property
metadata and catalog
* converter for handling Iceberg-specific configurations.
+ *
+ * @param config the Gravitino connector configuration
*/
- public IcebergConnectorAdapter() {
+ public IcebergConnectorAdapter(GravitinoConfig config) {
this.propertyMetadata = new IcebergPropertyMeta();
this.catalogConverter = new IcebergCatalogPropertyConverter();
+ this.config = config;
}
@Override
public Map<String, String> buildInternalConnectorConfig(
GravitinoCatalog catalog, Credential[] credentials) throws Exception {
- Map<String, String> config =
+ // The catalog backend describes how Gravitino stores the metadata; it
does not decide how
+ // Trino reaches the data. Whenever an Iceberg REST server endpoint is
available for this
+ // catalog's metalake, the catalog is loaded through it, the only path
that supports temporary
+ // credentials. A catalog that already has a REST backend keeps pointing
at its own configured
+ // endpoint. If no endpoint is available, this falls back to translating
catalog-backend as
+ // before — nothing to configure either way.
+ //
+ // The manual override is plain local config, so it is valid on every node
as-is. The
+ // discovered endpoint is coordinator-only knowledge, so it is read from
the catalog's own
+ // properties, where the coordinator embeds it at registration time (see
+ // embedDiscoveredIcebergRestUri), rather than from GravitinoConfig
directly.
+ String restUri = config.getManualIcebergRestUri();
Review Comment:
The manual URI is global and wins for every catalog. In multi-metalake mode
one IRC still serves exactly one metalake, so setting this override for
metalake A also routes metalake B catalogs to A and causes REST lookup
failures. Please scope the manual override to a metalake, provide per-metalake
overrides, or reject this configuration in multi-metalake mode, with an A/B
routing test.
--
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]
