[#12440] improvement(core): Replace the entity change log listener retry/EXIT policy with a cache-clear fallback

[yuqi1129]

What changes were proposed in this pull request?

Make "dispatch once, always advance the cursor, listeners are self-healing" the entity change log contract, and let each listener recover locally.

Poller and configs:

• EntityChangeLogPoller: remove ListenerFailureAction, exitHandler, pendingDelivery, BatchDelivery.retryOnly/attempts and handleExhaustedRetries. Each batch is dispatched once and the cursor always advances; every listener failure is logged at ERROR. The self-healing contract is stated in the poller and EntityChangeLogListener javadoc, so a listener that cannot contain its own failures is not added silently.
• Remove gravitino.entityChangeLog.listenerMaxRetries and gravitino.entityChangeLog.listenerFailureAction from Configs, their wiring in RelationalEntityStore, and their entries in docs/gravitino-server-config.md.
• Clean up the removed config stubs in 13 test/benchmark classes.

All three registered listeners now recover by clearing the cache they maintain, which is a strict superset of the invalidation that failed and of the rest of the batch:

Listener	Cache	Recovery
EntityCacheChangeLogListener	entity cache	clears the whole cache (already did)
JcasbinChangeListener	metadataIdCache	clears the whole cache (new)
CatalogChangeLogListener	catalog cache	clears the whole cache (new)

Notes on the two listeners that changed:

• JcasbinChangeListener is a third change-log listener that the issue did not account for. It tolerated poison rows but propagated a failed invalidation, so under the dispatch-once contract its metadataIdCache would feed a stale name→id mapping to authorization decisions until the entry's TTL expired.
• CatalogChangeLogListener clears the catalog cache on a failed eviction. This is a deliberate tradeoff, documented in its javadoc: clearing closes the CatalogWrapper of every cached catalog, including catalogs this process is actively serving, so in-flight requests can hit NoClassDefFoundError from a closed IsolatedClassLoader (the failure mode of [#11736] fix(core): retry failed entity change log listeners and do not remove logs when polling. #11739). It is accepted so that a changed catalog is never served stale, and the clear runs only on a failed eviction, off the normal path. Malformed rows and a failed consumeLocalMutation probe are still skipped rather than escalated, since they name no eviction to recover.

Caches deliberately left alone: ownerRelCache is driven by JcasbinChangeListener's own owner_meta poller, whose cursor only advances after a successful invalidation batch, so it already retries; userRoleCache, groupRoleCache and loadedRoles are version-validated on every read and cannot go stale from a missed batch.

Why are the changes needed?

After #12374 every registered listener can recover locally, so the retry/EXIT path is effectively unreachable while carrying real cost:

1. EXIT trades the whole server for a condition a local cache clear already resolves. Killing a node to fix a stale cache entry is a heavy, surprising failure mode for operators.
2. A paused cursor blocks cache invalidation for every listener in the process while one listener retries, so a single misbehaving listener degrades cluster-wide coherence for up to 10 poll intervals.
3. The retained batch, pendingDelivery, BatchDelivery.retryOnly, attempts tracking and handleExhaustedRetries add machinery and two public configs for a path no listener reaches.

Fix: #12440

Does this PR introduce any user-facing change?

Yes:

• Removed config keys gravitino.entityChangeLog.listenerMaxRetries and gravitino.entityChangeLog.listenerFailureAction. Both are VERSION_2_0_0 and 2.0.0 is unreleased, so no deprecation cycle is needed.
• A node no longer stops itself (System.exit(1)) when a listener keeps failing to apply a change log batch.

How was this patch tested?

New and reworked unit tests:

• TestEntityChangeLogPoller: the four retry/pause/EXIT/SKIP cases are replaced by testThrowingListenerNeitherPausesCursorNorBlocksOtherListeners (each batch dispatched exactly once, the healthy listener sees every batch, the cursor advances past both) and testUnregisteredListenerIsSkipped.
• TestJcasbinChangePoller (7 → 14): the metadataIdCache clear fallback on prefix, leaf-key and batch-lock failures; a failed clear propagating to the poller; the happy path clearing nothing; ownerRelCache not cleared as collateral; plus leaf-vs-prefix keying, which had no coverage.
• TestCatalogChangeLogListener (3 → 7): the clear on a failed eviction; no clear on the happy path; malformed rows and a failed consumeLocalMutation probe skipped without clearing; a failed clear propagating.

Suites run locally: :core:test (1657 tests) and :server-common:test (272 tests) with --rerun-tasks, both green, plus the unit tests of the four catalog modules whose config stubs changed. :core:javadoc reports no new warnings on the touched files. Docker was not available locally, so docker-tagged tests and integration tests were not run.

[Copilot]

Pull request overview

This pull request changes the EntityChangeLogPoller contract to “dispatch once and always advance the cursor”, removes the retry/EXIT machinery and related configs, and shifts resilience to each listener via local self-healing (cache-clear fallbacks) so one failing listener can’t stall process-wide cache coherence.

Changes:

• Simplify EntityChangeLogPoller to deliver each batch once, log listener failures at ERROR, and always advance the cursor; remove retry/pause/EXIT/SKIP behavior and associated config keys/wiring.
• Update listeners (notably JcasbinChangeListener and CatalogChangeLogListener) to recover from invalidation failures by clearing their maintained caches, and update listener/poller JavaDoc accordingly.
• Update documentation and expand/adjust unit tests and test config stubs to reflect the new contract and removed configuration.

Reviewed changes

Copilot reviewed 26 out of 26 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
server-common/src/test/java/org/apache/gravitino/server/authorization/jcasbin/TestJcasbinChangePoller.java	Expands unit coverage for metadataIdCache invalidation behavior and cache-clear fallback scenarios.
server-common/src/main/java/org/apache/gravitino/server/authorization/jcasbin/JcasbinChangeListener.java	Adds “clear whole cache” fallback on invalidation failure under the dispatch-once contract and updates JavaDoc.
docs/gravitino-server-config.md	Removes the deleted listener retry/failure-action configuration entries from server config docs.
core/src/test/java/org/apache/gravitino/tag/TestTagManager.java	Removes stubs/imports for deleted entity-change-log listener retry/failure-action configs.
core/src/test/java/org/apache/gravitino/storage/relational/TestRelationalEntityStoreHierarchicalCache.java	Removes stubs for deleted entity-change-log listener retry/failure-action configs.
core/src/test/java/org/apache/gravitino/storage/relational/TestEntityChangeLogPoller.java	Reworks tests to assert dispatch-once behavior, cursor advancement, and unregister skipping semantics.
core/src/test/java/org/apache/gravitino/storage/relational/TestEntityCacheCrossNodeInvalidation.java	Updates poller construction to match new constructor/signature (no retry/failure-action args).
core/src/test/java/org/apache/gravitino/storage/AbstractEntityStorageTest.java	Removes stubs/imports for deleted entity-change-log listener retry/failure-action configs.
core/src/test/java/org/apache/gravitino/stats/TestStatisticManager.java	Removes stubs/imports for deleted entity-change-log listener retry/failure-action configs.
core/src/test/java/org/apache/gravitino/policy/TestPolicyManager.java	Removes stubs/imports for deleted entity-change-log listener retry/failure-action configs.
core/src/test/java/org/apache/gravitino/hook/TestFilesetHookDispatcher.java	Removes stubs/imports for deleted entity-change-log listener retry/failure-action configs.
core/src/test/java/org/apache/gravitino/catalog/TestCatalogChangeLogListener.java	Expands tests for catalog-cache clear fallback and propagation behavior under dispatch-once semantics.
core/src/test/java/org/apache/gravitino/authorization/TestOwnerManager.java	Removes stubs/imports for deleted entity-change-log listener retry/failure-action configs.
core/src/test/java/org/apache/gravitino/authorization/TestAccessControlManager.java	Removes stubs/imports for deleted entity-change-log listener retry/failure-action configs.
core/src/main/java/org/apache/gravitino/storage/relational/RelationalEntityStore.java	Removes wiring for deleted listener retry/failure-action configs when constructing the poller.
core/src/main/java/org/apache/gravitino/storage/relational/EntityChangeLogPoller.java	Implements dispatch-once + unconditional cursor advancement; removes retry/EXIT machinery and related state.
core/src/main/java/org/apache/gravitino/storage/relational/EntityChangeLogListener.java	Updates listener contract JavaDoc to require self-healing (batch is never replayed).
core/src/main/java/org/apache/gravitino/storage/relational/EntityChangeLogCleaner.java	Updates retention rationale comment to reflect backlog/slow-drain rather than paused-retry semantics.
core/src/main/java/org/apache/gravitino/storage/relational/EntityCacheChangeLogListener.java	Updates JavaDoc to reflect that poller logs and moves on if listener recovery ultimately fails (no retries).
core/src/main/java/org/apache/gravitino/Configs.java	Removes deleted config entries/constants and associated validation for listener retry/failure-action settings.
core/src/main/java/org/apache/gravitino/catalog/CatalogChangeLogListener.java	Changes catalog listener to clear whole catalog cache on eviction failure and documents the classloader tradeoff.
core/src/jmh/java/org/apache/gravitino/cache/it/AbstractEntityStorageBenchmark.java	Removes benchmark config stubs for deleted listener retry/failure-action configs.
catalogs/catalog-model/src/test/java/org/apache/gravtitino/catalog/model/TestModelCatalogOperations.java	Removes test config stubs for deleted listener retry/failure-action configs.
catalogs/catalog-lakehouse-generic/src/test/java/org/apache/gravitino/catalog/lakehouse/generic/TestGenericCatalogOperations.java	Removes test config stubs for deleted listener retry/failure-action configs.
catalogs/catalog-kafka/src/test/java/org/apache/gravitino/catalog/kafka/TestKafkaCatalogOperations.java	Removes test config stubs for deleted listener retry/failure-action configs.
catalogs/catalog-fileset/src/test/java/org/apache/gravitino/catalog/fileset/TestFilesetCatalogOperations.java	Removes test config stubs for deleted listener retry/failure-action configs.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Pull request overview

Copilot reviewed 26 out of 26 changed files in this pull request and generated no new comments.

Suppressed comments (1)

core/src/main/java/org/apache/gravitino/storage/relational/EntityCacheChangeLogListener.java:57

• The Javadoc says stale entity-cache entries may be served "until they expire", but cache expiration can be disabled (e.g. TTL set to 0), in which case entries may remain stale indefinitely. Consider rewording to avoid implying expiry is guaranteed.

 *       clear fails the exception propagates to {@link EntityChangeLogPoller}, which logs it and
 *       moves on: the batch is not retried, so this node may keep serving stale entries until they
 *       expire.

[github-actions]

Code Coverage Report

Overall Project	68.65% +0.19%	🟢
Files changed	86.83%	🟢

Module	Coverage
aliyun	1.72%	🔴
api	49.31%	🟢
authorization-common	85.96%	🟢
aws	42.04%	🟢
azure	2.47%	🔴
catalog-common	9.92%	🔴
catalog-fileset	79.77%	🟢
catalog-glue	68.95%	🟢
catalog-hive	79.4%	🟢
catalog-jdbc-common	45.7%	🟢
catalog-jdbc-doris	81.8%	🟢
catalog-jdbc-mysql	79.33%	🟢
catalog-jdbc-postgresql	83.39%	🟢
catalog-jdbc-starrocks	79.16%	🟢
catalog-kafka	77.01%	🟢
catalog-lakehouse-generic	59.18%	🟢
catalog-lakehouse-hudi	79.1%	🟢
catalog-lakehouse-iceberg	85.93%	🟢
catalog-lakehouse-paimon	84.23%	🟢
catalog-model	77.72%	🟢
cli	44.48%	🟢
client-java	78.46%	🟢
common	52.75%	🟢
core	83.6% +0.22%	🟢
filesystem-hadoop3	77.28%	🟢
flink	0.0%	🔴
flink-common	48.68%	🟢
flink-runtime	0.0%	🔴
gcp	14.12%	🔴
hadoop-auth	68.0%	🟢
hadoop-common	12.7%	🔴
hive-metastore-common	53.4%	🟢
iceberg-aliyun-bundle	0.0%	🔴
iceberg-common	64.75%	🟢
iceberg-rest-server	75.03%	🟢
idp-basic	86.02%	🟢
integration-test-common	0.0%	🔴
jobs	62.92%	🟢
lance-common	31.75%	🔴
lance-rest-server	63.47%	🟢
lineage	53.02%	🟢
optimizer	83.24%	🟢
optimizer-api	21.95%	🔴
server	87.25%	🟢
server-common	79.67% -0.85%	🟢
spark	28.57%	🔴
spark-common	45.89%	🟢
tencent	69.84%	🟢
trino-connector	40.29%	🟢

Files

Module	File	Coverage
core	EntityCacheChangeLogListener.java	100.0%	🟢
	Configs.java	99.04%	🟢
	CatalogChangeLogListener.java	94.83%	🟢
	EntityChangeLogCleaner.java	91.53%	🟢
	EntityChangeLogPoller.java	89.29%	🟢
	RelationalEntityStore.java	76.85%	🟢
	EntityChangeLogListener.java	0.0%	🔴
server-common	JcasbinChangeListener.java	63.64%	🟢

[Copilot]

Pull request overview

Copilot reviewed 26 out of 26 changed files in this pull request and generated 1 comment.

[jerryshao]

What's the meaning here "that failed and of the"?

[jerryshao]

What's the meaning of "name a remote mutation"? The comment is really hard to understand.

[Copilot]

Pull request overview

Copilot reviewed 26 out of 26 changed files in this pull request and generated no new comments.

[jerryshao]

What is the meaning of "names no catalog"?

[jerryshao]

Most of the comments are too vague to understand. You don't have to explain everything, just make clear about your changes, that should be enough.

[jerryshao]

A couple of correctness/robustness observations from a deeper pass on the self-healing contract this PR introduces.

[jerryshao]

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?

[jerryshao]

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?