Copilot commented on code in PR #12710:
URL: https://github.com/apache/gravitino/pull/12710#discussion_r3878989691
##########
trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/catalog/CatalogConnectorManager.java:
##########
@@ -207,6 +215,35 @@ private void loadMetalake() {
}
}
+ /**
+ * Asks the Gravitino server whether it has an Iceberg REST server running
for this metalake, and
+ * caches the answer on the shared {@link GravitinoConfig} for {@code
IcebergConnectorAdapter} to
+ * read on the next catalog load. Failures — including talking to a
Gravitino server older than
+ * this endpoint — must not interrupt catalog loading, so they are swallowed
here; Iceberg
+ * catalogs simply keep their last known routing decision until the next
successful poll. A
+ * failure is logged at ERROR on every poll because routing through Iceberg
REST is required when
+ * enabled. Catalog loading continues so that unrelated catalogs remain
available.
+ */
+ private void refreshIcebergRestUri(String metalakeName) {
+ try {
+ config.setDiscoveredIcebergRestUri(
+ metalakeName,
gravitinoClient.icebergRestServiceUri(metalakeName).orElse(null));
+ if (icebergRestDiscoveryFailing.remove(metalakeName)) {
+ LOG.info("Iceberg REST service discovery for metalake {} recovered.",
metalakeName);
+ }
+ } catch (Exception e) {
+ icebergRestDiscoveryFailing.add(metalakeName);
+ LOG.error(
+ "Failed to query the Iceberg REST service endpoint for metalake {};
Iceberg catalogs "
+ + "without a configured REST endpoint cannot be registered until
discovery "
+ + "recovers. Set gravitino.iceberg.rest-uri explicitly, upgrade
the Gravitino "
+ + "server to one that supports discovery, or disable Iceberg
REST routing with "
+ + "gravitino.iceberg.rest-routing-enabled=false to use legacy
backend translation.",
+ metalakeName,
+ e);
+ }
+ }
Review Comment:
The Javadoc/field comment says Iceberg REST discovery failures should only
be logged at WARN on the transition into/out of failure, but the current
implementation logs an ERROR on every refresh poll. This can spam logs in a
steady failure mode; consider logging once on first failure and downgrading
subsequent polls to DEBUG until recovery.
##########
trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/GravitinoConfig.java:
##########
@@ -596,16 +637,42 @@ public String toCatalogConfig() {
continue;
}
String value = config.get(entry.getKey());
- if (value != null) {
+ if (value != null
+ &&
!GravitinoConnectorFactory.isSecuritySensitivePropertyName(entry.getKey())) {
stringList.add(String.format("\"%s\"='%s'", entry.getKey(), value));
}
}
- // copy the configuration by the prefix of GRAVITINO_CLIENT_CONFIG_PREFIX
+ // copy the configuration by the prefix of GRAVITINO_CLIENT_CONFIG_PREFIX
and
+ // GRAVITINO_ICEBERG_REST_CATALOG_CONFIG_PREFIX
config.entrySet().stream()
- .filter(entry ->
entry.getKey().startsWith(GRAVITINO_CLIENT_CONFIG_PREFIX.key))
+ .filter(
+ entry ->
+ (entry.getKey().startsWith(GRAVITINO_CLIENT_CONFIG_PREFIX.key)
+ || entry
+ .getKey()
+
.startsWith(GRAVITINO_ICEBERG_REST_CATALOG_CONFIG_PREFIX.key))
+ &&
!GravitinoConnectorFactory.isSecuritySensitivePropertyName(entry.getKey()))
.forEach(
entry ->
stringList.add(String.format("\"%s\"='%s'", entry.getKey(),
entry.getValue())));
+ config.entrySet().stream()
+ .filter(entry ->
entry.getKey().startsWith(GRAVITINO_DYNAMIC_CATALOG_ENV_PREFIX))
+ .forEach(
+ entry -> {
+ String propertyName =
+
entry.getKey().substring(GRAVITINO_DYNAMIC_CATALOG_ENV_PREFIX.length());
+ String environmentVariable = entry.getValue();
+ if (propertyName.isEmpty()
+ ||
!ENVIRONMENT_VARIABLE_NAME.matcher(environmentVariable).matches()) {
Review Comment:
Dynamic-catalog env var mappings are rendered into a SQL `CREATE CATALOG`
statement using the unvalidated `propertyName` substring. If a misconfigured
key contains quotes or other special characters, it can break the statement
(and potentially allow SQL injection into the generated command).
Validate/sanitize `propertyName` before embedding it into SQL, similar to the
existing environment variable name validation.
--
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]
