PHOENIX-7978 Harden replay/forward poll scheduling against wall-clock… - #2598
PHOENIX-7978 Harden replay/forward poll scheduling against wall-clock…#2598Himanshu-g81 wants to merge 2 commits into
Conversation
… and scheduler drift The round-eligibility gate for replication replay/forward becomes eligible when currentTime - lastRoundEndTimestamp >= roundTimeMills + bufferMillis, evaluated on the wall clock. PHOENIX-7813 aligned the scheduler wake to that grid, but the wake is fired on the monotonic clock (System.nanoTime) and was computed with zero margin, so small nanoTime-vs-wall-clock drift could tip a wake just below the boundary and the region server would lose a full (~60s) cycle. Most damaging during planned failover. Two fixes, both in the shared base class ReplicationLogDiscovery (inherited by ReplicationLogDiscoveryReplay and ReplicationLogDiscoveryForwarder): - Epsilon margin on the aligned wake instant: anchor the delay at bufferMillis + epsilon (via Math.floorMod) so the wake lands just after the eligibility boundary rather than exactly on it. New config phoenix.replication.discovery.aligned.delay.epsilon.millis (default 500). - Per-cycle re-anchor: replace scheduleAtFixedRate with a self-rescheduling one-shot chain that recomputes the aligned delay every cycle, re-pinning each wake to the wall-clock grid instead of letting a one-time misalignment persist. Uses a ScheduledThreadPoolExecutor with setExecuteExistingDelayedTasksAfterShutdownPolicy(false) so stop() is deterministic. Each replay cycle is bound to the scheduler generation it was launched on and reschedules only if isRunning && owner == scheduler, preventing a stale in-flight cycle from grafting a second chain onto a new scheduler after a stop()->start() restart (which would otherwise double the effective poll rate). Testing: ReplicationLogDiscoveryTest 48/48 (incl. stale-generation, start()-rollback, and replay/reschedule error-swallow regressions); ReplicationLogDiscoveryReplayTestIT 48/48; StoreAndForwardFailoverIT 1/1; spotless:check green on phoenix-core and phoenix-core-server.
73bb299 to
72951b0
Compare
There was a problem hiding this comment.
Pull request overview
Hardens replay/forward polling alignment against clock drift and restart-related duplicate scheduling.
Changes:
- Adds configurable epsilon-based wake alignment.
- Replaces fixed-rate polling with generation-bound one-shot rescheduling.
- Expands lifecycle, alignment, and failure-path tests.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
ReplicationLogDiscovery.java |
Implements epsilon alignment and self-rescheduling. |
ReplicationLogDiscoveryTest.java |
Tests scheduling, restart, and alignment behavior. |
ReplicationLogDiscoveryReplayTestIT.java |
Removes obsolete fixed-interval tests. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| } catch (Throwable t) { | ||
| // Any other failure (e.g. a bad epsilon config value making | ||
| // computeAlignedInitialDelay throw) would otherwise be swallowed by the executor | ||
| // into the discarded Future and silently wedge the polling chain with | ||
| // isRunning==true -- the exact silent-stop this class is meant to prevent. |
| */ | ||
| @GuardedBy("this") | ||
| protected void scheduleNextReplay() { | ||
| long delayMs = computeAlignedInitialDelay(); |
There was a problem hiding this comment.
Added [0, roundTimeMills) range validation plus a NumberFormatException catch, clamping any bad value to the default with a one-shot WARN. Chose clamp-over-reject because Math.floorMod already keeps the delay valid, so throwing would only wedge the group in the ~60s supervisor-retry loop for a benign misconfig.
| public long getAlignedDelayEpsilonMillis() { | ||
| return conf.getLong(REPLICATION_ALIGNED_DELAY_EPSILON_MILLIS_KEY, | ||
| DEFAULT_ALIGNED_DELAY_EPSILON_MILLIS); | ||
| } |
| } catch (Throwable t) { | ||
| LOG.error("Error during replay for haGroup: {}", haGroupName, t); |
There was a problem hiding this comment.
Narrowed the replay() catch to Exception (chain continues) and added a separate catch (Error) that logs the otherwise-swallowed fatal and rethrows without rescheduling — with the reschedule moved out of finally so it can't fire during propagation. The Error path also marks the service not-running and shuts the executor down, so the replay-service supervisor rebuilds a fresh one instead of looping on a corrupted JVM.
| protected void runReplayCycle(ScheduledExecutorService owner) { | ||
| try { | ||
| replay(); | ||
| } catch (Throwable t) { |
There was a problem hiding this comment.
isRunning remains true and the executor stays alive, so discovery.isRunning() reports healthy while nothing happens.
At least set isRunning = false in the catch handler.
Another option is to log a warning at WARN log level and fall back to DEFAULT_ALIGNED_DELAY_EPSILON_MILLIS on NumberFormatException (or anything that is not RejectedExecutionException) and retry. This rides over configuration errors.
There was a problem hiding this comment.
Addressed in latest commit.
The reschedule catch now sets isRunning=false and shuts the owner executor down, so the ReplicationLogReplayService fixed-rate supervisor restarts it (start-time/config failures were already rolled back in start()).
| } catch (Throwable t) { | ||
| LOG.error("Error during replay for haGroup: {}", haGroupName, t); |
| // swaps in a new scheduler; a cycle launched on the old one must reschedule onto | ||
| // that same (now shut-down) scheduler, not the new one. | ||
| ScheduledExecutorService owner = scheduler; | ||
| LOG.info("Scheduling next replay for haGroup: {} in {}ms", haGroupName, delayMs); |
There was a problem hiding this comment.
No.
Audit all your code for this kind of noisy and low value INFO level logging.
It must be DEBUG, if not TRACE.
There was a problem hiding this comment.
Downgraded this one to DEBUG. I will review all the pre-existing once and will sweep the once which might be noise (based on experience with debugging during initial rounds of testing) to DEBUG/TRACE in a separate follow-up to keep scope of this PR limited, if that sounds good to you? or can fold that in here if you'd prefer.
…ilon validation - Self-heal on mid-chain reschedule failure: if scheduleNextReplay() throws after a healthy start, mark the discovery not-running and shut the executor down so the ReplicationLogReplayService supervisor rebuilds it, instead of leaving isRunning=true with an idle executor that wedges polling until an RS restart. - Guard against re-selecting the just-processed grid point: track lastAlignedTargetMillis and advance one round when a wake lands on/before it; derive the delay and the absolute target from a single clock read. - Validate the aligned-delay epsilon: clamp non-numeric / out-of-range [0, roundTimeMills) values to the default with a one-shot WARN rather than throwing (which would wedge the group in the supervisor-retry loop). - Split the replay() catch: Exception logs and continues the chain; Error is logged (it would otherwise vanish into the discarded Future), tears the chain down, and is rethrown; the reschedule moved out of the finally block. - Downgrade the per-cycle "Scheduling next replay" INFO line to DEBUG.
dfa1246 to
5b550f9
Compare
Problem
The replay/forward round-eligibility gate is evaluated on the wall clock, but PHOENIX-7813
aligned the scheduler wake — fired on the monotonic clock (
System.nanoTime) — to that boundarywith zero margin. Small nanoTime-vs-wall-clock drift can tip a wake just below the boundary,
so the round isn't yet eligible and the region server loses a full (~60s) cycle. Most damaging
during planned failover.
Fix
Both changes live in the shared base class
ReplicationLogDiscovery(inherited byReplicationLogDiscoveryReplayandReplicationLogDiscoveryForwarder):exactly on it. New config
phoenix.replication.discovery.aligned.delay.epsilon.millis(default500).scheduleAtFixedRatewith a self-rescheduling one-shot thatrecomputes the aligned delay every cycle, so drift can't accumulate. Each cycle is bound to its
scheduler generation to avoid double-scheduling after a
stop()→start()restart.