jerryshao commented on code in PR #12445:
URL: https://github.com/apache/gravitino/pull/12445#discussion_r3783417758
##########
core/src/main/java/org/apache/gravitino/storage/relational/EntityChangeLogPoller.java:
##########
@@ -410,39 +307,29 @@ private List<EntityChangeLogListener>
notifyListeners(BatchDelivery delivery) {
delivery.firstChangeId(),
delivery.lastChangeId);
} catch (Exception e) {
Review Comment:
`catch (Exception e)` here (and in `pollChanges()` at line 176) only catches
`Exception`, not `Error`. `scheduleWithFixedDelay`'s contract is that any
uncaught `Throwable` from a periodic task silently and permanently cancels all
future executions. Notably, `CatalogChangeLogListener`'s javadoc in this same
PR names `NoClassDefFoundError` as an accepted failure mode of its
whole-cache-clear fallback (the #11739 failure mode) — so a listener can throw
an `Error` under exactly the recovery path this PR introduces, which would kill
the poller thread for *every* listener, not just the failing one. That
contradicts the "self-healing" contract this class's own javadoc asserts
(listener failures are always logged and the cursor always advances). Worth
catching `Throwable` here, or documenting why `Error` is intentionally out of
scope?
##########
core/src/main/java/org/apache/gravitino/catalog/CatalogChangeLogListener.java:
##########
@@ -59,51 +72,87 @@ public CatalogChangeLogListener(CatalogManager
catalogManager) {
@Override
public void onEntityChange(List<EntityChangeRecord> changes) {
+ List<CatalogInvalidation> remoteInvalidations = new ArrayList<>();
for (EntityChangeRecord change : changes) {
+ if (!isCatalogChange(change)) {
+ continue;
+ }
+
+ Optional<NameIdentifier> identOpt = catalogIdentifier(change);
Review Comment:
`catalogIdentifier(change)` is called here with no enclosing try/catch. The
old code wrapped identifier resolution + `consumeLocalMutation` + `invalidate`
for a record in one `catch (RuntimeException)`, isolating any unexpected
failure to a single row. Today this is safe because `catalogIdentifier()` only
catches `IllegalArgumentException` internally, but that safety now depends
entirely on an implementation detail of `decode()`/`NameIdentifier.of()` two
calls down, with no defensive boundary at this call site. If a future change to
that codec throws a different unchecked exception, `onEntityChange()` would
abort for the whole batch — skipping the self-heal cache-clear for every
already-collected `remoteInvalidations` — rather than being isolated to one bad
row, as the class javadoc claims ("a malformed row is skipped ... and leaves
nothing stale"). Worth wrapping this call (or the loop body) defensively?
--
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]
