-
Notifications
You must be signed in to change notification settings - Fork 876
[#11736] improvement(core): Close EntityChangeLogPoller commit-ordering gap and listener-failure cursor advance #11739
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
a1ffcff
e850679
648a8c3
47a831c
2192fd2
262b699
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,6 +18,10 @@ | |
| */ | ||
| package org.apache.gravitino.storage.relational; | ||
|
|
||
| import static org.apache.gravitino.Configs.DEFAULT_ENTITY_CHANGE_LOG_CLEANUP_INTERVAL_SECS; | ||
| import static org.apache.gravitino.Configs.DEFAULT_ENTITY_CHANGE_LOG_POLL_LAG_SECS; | ||
| import static org.apache.gravitino.Configs.DEFAULT_ENTITY_CHANGE_LOG_RETENTION_SECS; | ||
|
|
||
| import com.google.common.annotations.VisibleForTesting; | ||
| import com.google.common.base.Preconditions; | ||
| import java.util.Collections; | ||
|
|
@@ -38,8 +42,22 @@ | |
| * | ||
| * <p>The poller owns the single high-water mark for a Gravitino server process and dispatches each | ||
| * consumed batch to registered listeners. Listeners should only perform idempotent local cache | ||
| * invalidation. The cursor always advances after dispatch regardless of individual listener | ||
| * failures, so a faulty listener cannot block other listeners or prevent pruning. | ||
| * invalidation. Listener failures are isolated to polling progress: a failing listener is logged | ||
| * and does not stop the Gravitino server or prevent the poller from invoking the other listeners in | ||
| * the same cycle. However, because the process owns one shared cursor, any listener failure pauses | ||
| * forward progress: the cursor is <b>not</b> advanced past the batch, so newer change rows are not | ||
| * consumed until the failed batch is retried and every listener succeeds within the retention | ||
| * window. If a listener keeps failing, the poller keeps retrying the blocked batch until the | ||
| * listener recovers or the records age out by retention cleanup. Because invalidation is | ||
| * idempotent, re-dispatching an already-applied batch to a healthy listener is harmless. | ||
| * | ||
| * <p>The poller also applies a lagging high-water mark: it only consumes change rows whose {@code | ||
| * created_at} is at least {@code pollLagMs} in the past (by the database clock). Auto-increment ids | ||
| * are assigned at INSERT but become visible at COMMIT, so a lower id can commit after a higher id | ||
| * and be skipped by an id-only cursor. Because a smaller-id row was necessarily inserted no later, | ||
| * waiting until a row is {@code pollLagMs} old gives that row's inserting transaction time to | ||
| * commit before the cursor moves past it. As long as {@code pollLagMs} exceeds the longest write | ||
| * transaction, this prevents the commit-ordering gap from dropping an invalidation permanently. | ||
| */ | ||
| public class EntityChangeLogPoller implements AutoCloseable { | ||
|
|
||
|
|
@@ -52,6 +70,7 @@ public class EntityChangeLogPoller implements AutoCloseable { | |
| private final long pollIntervalSecs; | ||
| private final long retentionMs; | ||
| private final long cleanupIntervalMs; | ||
| private final long pollLagMs; | ||
| private final LongSupplier clockMs; | ||
|
|
||
| private ScheduledExecutorService scheduler; | ||
|
|
@@ -66,8 +85,9 @@ public class EntityChangeLogPoller implements AutoCloseable { | |
| public EntityChangeLogPoller(long pollIntervalSecs) { | ||
| this( | ||
| pollIntervalSecs, | ||
| TimeUnit.DAYS.toMillis(1), | ||
| TimeUnit.HOURS.toMillis(1), | ||
| TimeUnit.SECONDS.toMillis(DEFAULT_ENTITY_CHANGE_LOG_RETENTION_SECS), | ||
| TimeUnit.SECONDS.toMillis(DEFAULT_ENTITY_CHANGE_LOG_CLEANUP_INTERVAL_SECS), | ||
| TimeUnit.SECONDS.toMillis(DEFAULT_ENTITY_CHANGE_LOG_POLL_LAG_SECS), | ||
| System::currentTimeMillis); | ||
| } | ||
|
|
||
|
|
@@ -77,20 +97,38 @@ public EntityChangeLogPoller(long pollIntervalSecs) { | |
| * @param pollIntervalSecs interval between successive polling cycles | ||
| * @param retentionMs entity change retention in milliseconds, or 0 to disable cleanup | ||
| * @param cleanupIntervalMs interval between successive cleanup attempts in milliseconds | ||
| * @param pollLagMs lag in milliseconds; only rows older than this (by the DB clock) are consumed, | ||
| * or 0 to disable the lag | ||
| */ | ||
| public EntityChangeLogPoller(long pollIntervalSecs, long retentionMs, long cleanupIntervalMs) { | ||
| this(pollIntervalSecs, retentionMs, cleanupIntervalMs, System::currentTimeMillis); | ||
| public EntityChangeLogPoller( | ||
| long pollIntervalSecs, long retentionMs, long cleanupIntervalMs, long pollLagMs) { | ||
| this(pollIntervalSecs, retentionMs, cleanupIntervalMs, pollLagMs, System::currentTimeMillis); | ||
| } | ||
|
|
||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor: Preconditions error message omits the actual values. "retentionMs must be greater than pollLagMs when cleanup is enabled"When this fires at startup the operator sees an String.format(
"retentionMs (%d) must be greater than pollLagMs (%d) when cleanup is enabled",
retentionMs, pollLagMs)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK |
||
| @VisibleForTesting | ||
| EntityChangeLogPoller( | ||
| long pollIntervalSecs, long retentionMs, long cleanupIntervalMs, LongSupplier clockMs) { | ||
| long pollIntervalSecs, | ||
| long retentionMs, | ||
| long cleanupIntervalMs, | ||
| long pollLagMs, | ||
| LongSupplier clockMs) { | ||
| Preconditions.checkArgument(pollIntervalSecs > 0, "pollIntervalSecs must be positive"); | ||
| Preconditions.checkArgument(retentionMs >= 0, "retentionMs must be non-negative"); | ||
| Preconditions.checkArgument(cleanupIntervalMs > 0, "cleanupIntervalMs must be positive"); | ||
| Preconditions.checkArgument(pollLagMs >= 0, "pollLagMs must be non-negative"); | ||
| // A row becomes eligible for polling once it is pollLagMs old and eligible for pruning once it | ||
| // is retentionMs old. If cleanup is enabled (retentionMs > 0) but retentionMs is not strictly | ||
| // greater than pollLagMs, a row could be pruned before it is ever polled, silently dropping its | ||
| // invalidation. | ||
| Preconditions.checkArgument( | ||
| retentionMs == 0 || retentionMs > pollLagMs, | ||
| "retentionMs (%s) must be greater than pollLagMs (%s) when cleanup is enabled", | ||
| retentionMs, | ||
| pollLagMs); | ||
| this.pollIntervalSecs = pollIntervalSecs; | ||
| this.retentionMs = retentionMs; | ||
| this.cleanupIntervalMs = cleanupIntervalMs; | ||
| this.pollLagMs = pollLagMs; | ||
| this.clockMs = clockMs; | ||
| } | ||
|
|
||
|
|
@@ -183,22 +221,41 @@ private synchronized void doPollChanges() { | |
| } | ||
|
|
||
| List<EntityChangeRecord> dispatchedChanges = Collections.unmodifiableList(changes); | ||
| boolean allListenersSucceeded = true; | ||
| for (EntityChangeLogListener listener : listeners) { | ||
| try { | ||
| listener.onEntityChange(dispatchedChanges); | ||
| } catch (Exception e) { | ||
| allListenersSucceeded = false; | ||
| LOG.warn("Entity change listener {} failed", listener.getClass().getName(), e); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Concern: With the new cursor semantics, a listener failure blocks forward progress for ALL new change rows. If a listener fails on every cycle, the cache silently serves stale data until it recovers — or until retention cleanup prunes the stuck rows, at which point the invalidations are permanently lost. A single Consider escalating to
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm hesitant about these two levels and adopt the suggestion. |
||
| } | ||
| } | ||
|
|
||
| entityPollHighWaterId = maxSeenId; | ||
| // Only advance the cursor when every listener applied the batch. A listener failure must not | ||
| // drop the batch's invalidations: keeping the cursor in place re-dispatches the same batch on | ||
| // the next cycle until all listeners succeed. Listeners are idempotent, so re-dispatching to an | ||
| // already-applied listener is harmless. | ||
| if (allListenersSucceeded) { | ||
| entityPollHighWaterId = maxSeenId; | ||
| } else { | ||
| // The cursor cannot advance until every listener applies the batch, so forward progress is | ||
| // paused and the same batch is re-dispatched every cycle. If this persists, the stuck rows | ||
| // will eventually be pruned by retention cleanup and their invalidations lost permanently, | ||
| // leaving caches to serve stale data. Surface this at ERROR so operators can act. | ||
| LOG.error( | ||
| "Entity change cursor is paused at id {} because at least one listener failed to apply " | ||
| + "the current batch; invalidations will be lost if this is not resolved before the " | ||
| + "stuck rows age past the retention window", | ||
| entityPollHighWaterId); | ||
| } | ||
| pruneExpiredChangesIfNeeded(); | ||
| } | ||
|
|
||
| private List<EntityChangeRecord> fetchEntityChanges() { | ||
| return SessionUtils.getWithoutCommit( | ||
| EntityChangeLogMapper.class, | ||
| m -> m.selectEntityChanges(entityPollHighWaterId, ENTITY_CHANGE_POLLER_MAX_ROWS)); | ||
| m -> | ||
| m.selectEntityChanges(entityPollHighWaterId, pollLagMs, ENTITY_CHANGE_POLLER_MAX_ROWS)); | ||
| } | ||
|
|
||
| private static boolean handleInterruptIfAny(Throwable e, String context) { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Style: hardcoded
1should referenceDEFAULT_ENTITY_CHANGE_LOG_POLL_LAG_SECS.This repeats the magic value that is also defined in
Configs.DEFAULT_ENTITY_CHANGE_LOG_POLL_LAG_SECS. If the default is ever changed inConfigs, this single-arg constructor is silently left inconsistent.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, it makes sense.