Copilot commented on code in PR #12547:
URL: https://github.com/apache/gravitino/pull/12547#discussion_r3913107159
##########
trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/catalog/CatalogConnectorManager.java:
##########
@@ -206,12 +238,126 @@ private void loadMetalake() {
icebergRestUriDiscovery.refresh(usedMetalake, config,
gravitinoClient);
loadCatalogs(metalake);
} catch (Exception e) {
- LOG.error(e, "Load Metalake %s failed.", usedMetalake);
+ recordMetalakeError(usedMetalake, e);
}
}
- } catch (Exception e) {
- LOG.error(e, "Error when loading metalake");
+
+ pruneMissingMetalakes(usedMetalakes);
+
+ if (metalakeErrors.isEmpty()) {
+ recordLoadSuccess();
+ } else {
+ // Some metalake failed. The loop reaching its last line is not a
health signal, so do not
+ // advance the success time or clear the error, or load_status would
report a healthy loop
+ // while no catalog is being registered at all.
+ recordLoadFailure(
+ String.format(
+ "%d of %d metalakes failed to load: %s",
+ metalakeErrors.size(), usedMetalakes.size(), new
TreeMap<>(metalakeErrors)),
+ null);
+ }
+ } catch (Throwable t) {
+ // Catch Throwable, not Exception: scheduleWithFixedDelay silently
cancels the task forever
+ // the first time the runnable throws, and loading a Trino connector
plugin can raise
+ // NoClassDefFoundError. A dead loop must not look like a healthy one.
+ recordLoadFailure(toErrorMessage(t), t);
+ }
+ }
+
+ private void pruneMissingMetalakes(Set<String> usedMetalakes) {
+ // A metalake that was deleted, or that dropped out of the configuration,
leaves its catalog
+ // connectors, catalog rows and cached metalake handle behind. Without
this its catalogs keep
+ // reporting REGISTERED and stay live in Trino even though they no longer
exist, and
+ // getUsedMetalakes()/getMetalake() keep returning a metalake that isn't
loaded anymore.
+ for (Map.Entry<String, CatalogConnectorContext> entry :
catalogConnectors.entrySet()) {
+ GravitinoCatalog catalog = entry.getValue().getCatalog();
+ if (usedMetalakes.contains(catalog.getMetalake())) {
+ continue;
+ }
+ try {
+ unloadCatalog(catalog);
+ } catch (Exception e) {
+ // The metalake is gone but the catalog is still registered in Trino.
Record it, or the
+ // pruning below would drop the row and the table would report nothing
at all about a
+ // catalog that still shows up in SHOW CATALOGS.
+ recordCatalogState(
+ CatalogRegistrationState.failed(
+ catalog.getMetalake(),
+ catalog.getName(),
+ entry.getKey(),
+ catalog.getProvider(),
+ "The metalake was removed but the catalog could not be
unregistered from Trino: "
+ + toErrorMessage(e)),
+ e);
+ }
}
+
+ catalogStates.values().removeIf(state ->
!usedMetalakes.contains(state.getMetalake()));
Review Comment:
This unconditional pruning also removes the FAILED state recorded just above
when unregistering a catalog from a vanished metalake fails. The connector then
remains visible in `SHOW CATALOGS`, but `catalog_status` loses its diagnostic
row and `load_status` can report success. Retain states while their Trino
connector still exists, as the per-catalog pruning path already does.
##########
trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/GravitinoConnectorFactory.java:
##########
@@ -134,7 +133,6 @@ public Connector create(
// failed client initialization leaves a shared manager with a null
Gravitino client,
// causing later connector creation attempts to fail with a
misleading NPE.
catalogConnectorManager = newCatalogConnectorManager;
Review Comment:
The manager is published before `start()` succeeds, and the start-attempt
flag remains true if `start()` throws. A later static `create()` therefore
skips startup and returns a system connector even though no load task was
scheduled—the new tests explicitly exercise that dead-connector path. Preserve
and rethrow the initialization failure, or discard/close the failed manager so
creation cannot succeed with a stopped load loop.
##########
trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/GravitinoConfig.java:
##########
@@ -350,7 +350,10 @@ public String getURI() {
* @return the metalake name for used
*/
public String getMetalake() {
- return config.getOrDefault(GRAVITINO_METALAKE.key,
GRAVITINO_METALAKE.defaultValue);
+ // Trimmed so a stray leading/trailing space in the catalog properties
file does not make this
+ // value silently stop matching the canonical metalake name the load loop
records states
+ // under, e.g. in the catalog_status/load_status system tables'
per-metalake filtering.
+ return config.getOrDefault(GRAVITINO_METALAKE.key,
GRAVITINO_METALAKE.defaultValue).trim();
Review Comment:
The new whitespace-normalization behavior has no corresponding test in
`TestGravitinoConfig`. Add a case with leading/trailing whitespace to lock down
the returned canonical metalake and prevent this system-table filtering fix
from regressing.
##########
trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/catalog/CatalogRegister.java:
##########
@@ -348,15 +364,35 @@ static String redactSecrets(String createCatalogCommand) {
// Some JDBC drivers echo the failing statement back in the exception
message, which for a
// failed CREATE CATALOG would otherwise carry its embedded credentials into
every caller and
- // log line downstream. The original exception is deliberately not kept as
the cause, since a
- // logged stack trace prints causes' messages too and would defeat the
redaction.
+ // log line downstream. Every level of the cause chain is rebuilt with its
message redacted,
+ // rather than discarded outright, so a deep cause unrelated to credentials
(e.g. a connector's
+ // own configuration validation error) still reaches the caller.
private static SQLException redactedSqlException(SQLException e) {
- String message = e.getMessage() == null ? null :
redactSecrets(e.getMessage());
- SQLException redacted = new SQLException(message, e.getSQLState(),
e.getErrorCode());
+ Throwable redactedCause = e.getCause() == null ? null :
redactThrowableChain(e.getCause());
+ SQLException redacted =
+ new SQLException(
+ e.getMessage() == null ? null : redactSecrets(e.getMessage()),
+ e.getSQLState(),
+ e.getErrorCode(),
+ redactedCause);
redacted.setStackTrace(e.getStackTrace());
return redacted;
}
+ private static Throwable redactThrowableChain(Throwable t) {
+ // Guards against a cause cycle the same way toErrorMessage() in
CatalogConnectorManager does.
+ Throwable cause = t.getCause();
+ Throwable redactedCause = (cause == null || cause == t) ? null :
redactThrowableChain(cause);
Review Comment:
This guard handles only a throwable whose cause is itself. Java also permits
multi-node cycles such as A → B → A, which recurse here until
`StackOverflowError` instead of producing the intended redacted exception.
Track visited throwables by identity (or rebuild the chain iteratively) and
stop at any repeated node.
--
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]
