Copilot commented on code in PR #12555:
URL: https://github.com/apache/gravitino/pull/12555#discussion_r3829502813
##########
trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/catalog/iceberg/IcebergCatalogPropertyConverter.java:
##########
@@ -152,6 +172,102 @@ private Map<String, String>
buildJDBCBackendProperties(Map<String, String> prope
return jdbcProperties;
}
+ /**
+ * Builds the Trino Iceberg connector config that reaches this catalog
through the Gravitino
+ * Iceberg REST server, regardless of the catalog backend Gravitino uses to
store its metadata.
+ *
+ * <p>This is the only path on which credential vending works: the Iceberg
REST protocol issues a
+ * fresh temporary credential per table access, while Trino's jdbc and
hive_metastore Iceberg
+ * catalog types have nowhere to put the session token of an STS credential.
+ *
+ * @param catalog the Gravitino catalog to load
+ * @param gravitinoConfig the connector configuration holding the Iceberg
REST server endpoint
+ * @return the Trino Iceberg connector config
+ */
+ public Map<String, String> buildIcebergRestProperties(
+ GravitinoCatalog catalog, GravitinoConfig gravitinoConfig) {
+ String restUri = gravitinoConfig.getIcebergRestUri();
+ if (StringUtils.isBlank(restUri)) {
+ throw new TrinoException(
+ GravitinoErrorCode.GRAVITINO_MISSING_CONFIG,
+ "Missing required config 'gravitino.iceberg.rest-uri'. Set it to the
Gravitino Iceberg "
+ + "REST server endpoint, for example
http://localhost:9001/iceberg, or set "
+ + "'gravitino.iceberg.rest-enabled=false' to load Iceberg
catalogs through their "
+ + "catalog backend instead.");
+ }
+
+ Map<String, String> config = new HashMap<>();
+ // The order of put operations determines the priority of parameters.
+ config.putAll(buildStorageProperties(catalog.getProperties()));
+ config.put(TRINO_ICEBERG_REST_VENDED_CREDENTIALS, "true");
+ if (gravitinoConfig.isForwardUser()) {
+ config.put(TRINO_ICEBERG_REST_SESSION, "USER");
+ }
+ // The catalog's own trino.bypass properties override the defaults above,
so that a Trino
+ // release renaming one of them can be worked around without a connector
change.
+ config.putAll(super.gravitinoToEngineProperties(catalog.getProperties()));
+ // The Iceberg REST server endpoint and its authentication are
cluster-level operational
+ // settings, so they take precedence over anything set on a single catalog.
+ config.putAll(gravitinoConfig.getIcebergRestCatalogConfig());
Review Comment:
The comment about “order of put operations” is misleading with a `HashMap`
(iteration order is not guaranteed). What actually enforces priority here is
overwrite semantics (later `put/putAll` overrides earlier values for the same
keys). Update the comment to explicitly describe overwrite precedence, or
switch to an insertion-ordered map (e.g., `LinkedHashMap`) if you want
deterministic ordering for debugging/inspection.
##########
trino-connector/integration-test/src/test/java/org/apache/gravitino/trino/connector/integration/test/TrinoQueryITBase.java:
##########
@@ -46,6 +48,8 @@
public class TrinoQueryITBase {
private static final Logger LOG =
LoggerFactory.getLogger(TrinoQueryITBase.class);
+ private static final String GRAVITINO_ICEBERG_REST_PREFIX =
"gravitino.iceberg-rest.";
Review Comment:
This prefix constant is duplicated (also appears in `TrinoConnectorIT`).
Consider centralizing it (e.g., in `BaseIT` or a shared test utility) to avoid
drift if the aux-service config prefix ever changes.
--
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]
