PHOENIX-7992 Add replay-side performance metrics for HA replication - #2607
Open
Himanshu-g81 wants to merge 5 commits into
Open
PHOENIX-7992 Add replay-side performance metrics for HA replication#2607Himanshu-g81 wants to merge 5 commits into
Himanshu-g81 wants to merge 5 commits into
Conversation
Adds six standby replay-side performance metrics, each inheriting the
per-haGroup dimension from its metric source.
ReplicationLogDiscoveryReplay:
- endToEndReplayLagMs (histogram): round-eligible -> replay finished
- pickupLagMs (histogram): round-eligible -> file claimed (rename into
the in-progress directory)
ReplicationLogDiscovery (shared base):
- roundsExceedingRoundTime (counter): rounds whose new-file processing
duration exceeded roundTimeMills
ReplicationLogProcessor:
- mutationsReplayedCount (counter): replay throughput across all files
- mutationsPerFile (histogram): per-file mutation count, recorded only
for files that replayed at least one mutation so rotation-only files
do not skew the distribution toward zero
ReplicationLogTracker:
- markFileInProgressCollisionCount (counter): claim renames lost to a
competing process, a strict subset of the request count
The lag metrics use the round-eligible instant (round end + waiting
buffer) as the zero reference, so they exclude the fixed built-in wait
rather than measuring raw file age.
Tests: metric-source plumbing for all six metrics, plus recording-site
tests for the collision metric (rename-loses vs rename-wins) and the
slow-round metric (fast round vs slow round).
Record pickupLag only on a file's first claim, skipping in-progress reclaims that re-stamp the rename timestamp; anchor round-boundary files to the earlier round in getRoundEligibleTime; and rename two counters to match what they actually measure: mutationsReplayedCount -> successfulFileMutationsReplayedCount and markFileInProgressCollisionCount -> markFileInProgressRenameFailedCount. Adds ReplicationLogDiscoveryReplayProcessFileTest covering the eligibility math, first-claim-only pickup lag, and mid-replay failure handling.
…fy metric docs Follow-up addressing review feedback on apache#2607. Tests (C1): drive ReplicationLogProcessor.processLogFile end to end to guard the per-file mutation-metric recording sites, and add a mid-replay-failure case proving mutations applied before a failure are not counted (the writes sit before the catch). Assert the reset-immune successfulFileMutationsReplayedCount counter rather than the mutationsPerFile histogram max: MutableHistogram.getMax() is reset to 0 by the metrics system's periodic snapshot(), so a positive max is not stable to read after a multi-second real replay. Zero-mutation and failed files still assert a 0 histogram max, which is stable. Reclaim path (S1): convert the unchecked NumberFormatException from getFileTimestamp on a malformed in-progress file name into an IOException, so a single bad name is isolated to processOneRandomFile's per-file handler (marked failed and retry-counted) instead of escaping as a RuntimeException that aborts the whole in-progress sweep for the cycle. Adds a unit test. Docs: - roundsExceedingRoundTime lives on the shared discovery base, so clarify it is emitted on both the forwarder and replay sides, not replay-only (A5). - Note the endToEndReplayLag / pickupLag histograms are survivorship-biased (sampled only on success / first claim) and should be read alongside the failure and staleness signals (D1). - Note markFileInProgressRenameFailedCount counts only rename() == false, not an IOException thrown during the rename (D2). - Note endToEndReplayLag samples rotation-only / zero-mutation files that mutationsPerFile excludes, so the two cover different file populations (C2). - Fix stale processFile(Path) Javadoc to processFile(Path, boolean) (C4). - Log a breadcrumb when a first-claim file carries no rename timestamp (D4).
Follow-up to review feedback on apache#2607: replace the inline round-start floor in getRoundEligibleTime with ReplicationShardDirectoryManager.getNearestRoundStartTimestamp, which computes the identical floor. The boundary ternary (an on-boundary file belongs to the earlier round) is unchanged. ReplicationLogDiscoveryReplayProcessFileTest.newReplay now builds a real ReplicationShardDirectoryManager (its constructor only reads config and never touches the file system) instead of a mock, so the round math runs the production floor. With a mocked manager getNearestRoundStartTimestamp returned Mockito's default 0 and every lag sample was off by one full round.
getRoundEligibleTime anchored a file created exactly on a round boundary to the round that ENDS at that boundary (eligible = boundary + buffer). Rounds are half-open [start, end): getShardDirectory floors the timestamp, so a boundary file is stored in, and replayed by, the round that STARTS there. The old boundary ternary therefore under-counted such a file's eligibility by one full round, over-reporting its replay lag by up to roundTimeMills. Drop the ternary so eligibility is always roundStart + roundTimeMills + bufferMillis, where roundStart floors creationTs via ReplicationShardDirectoryManager.getNearestRoundStartTimestamp. Non-boundary files are unaffected. Update the boundary unit test (renamed to testFirstClaimOnRoundBoundaryAnchorsToStartingRound) and the method/class Javadoc to the half-open semantics.
There was a problem hiding this comment.
🟡 Changes recommended
Filename parsing now blocks otherwise replayable logs, while shared metrics also exceed the stated replay-only scope.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds HA replication replay performance metrics across discovery, tracking, and mutation processing.
Changes:
- Adds replay/pickup lag and slow-round metrics.
- Tracks successful mutations, mutations per file, and rename failures.
- Adds recording-site and metric-source tests.
File summaries
| File | Description |
|---|---|
ReplicationLogTrackerTest.java |
Tests rename-result metrics. |
ReplicationLogDiscoveryTest.java |
Tests slow-round recording. |
ReplicationLogDiscoveryForwarderTest.java |
Updates forwarding calls for claim state. |
ReplicationLogDiscoveryReplayProcessFileTest.java |
Tests replay lag recording. |
ReplicationReplayPerfMetricsTest.java |
Tests all six metric sources. |
ReplicationLogProcessorTestIT.java |
Verifies mutation metrics during replay. |
ReplicationLogTracker.java |
Records failed claim renames. |
ReplicationLogDiscoveryForwarder.java |
Accepts the claim-state argument. |
ReplicationLogDiscovery.java |
Propagates claim state and records slow rounds. |
ReplicationLogProcessor.java |
Records successful mutation throughput. |
ReplicationLogDiscoveryReplay.java |
Calculates replay and pickup lag. |
ReplicationLogTrackerMetricValues.java |
Exposes rename-failure values. |
ReplicationLogProcessorMetricValues.java |
Exposes mutation metrics. |
ReplicationLogDiscoveryMetricValues.java |
Exposes slow-round counts. |
MetricsReplicationLogTrackerImpl.java |
Implements rename-failure counter. |
MetricsReplicationLogTracker.java |
Defines rename-failure metric API. |
MetricsReplicationLogProcessorImpl.java |
Implements mutation metrics. |
MetricsReplicationLogProcessor.java |
Defines mutation metric API. |
MetricsReplicationLogDiscoveryReplayImpl.java |
Implements lag histograms. |
MetricsReplicationLogDiscoveryReplay.java |
Defines replay lag metrics. |
MetricsReplicationLogDiscoveryImpl.java |
Implements slow-round counter. |
MetricsReplicationLogDiscovery.java |
Defines slow-round metric API. |
Review details
- Files reviewed: 22/22 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // failure uses. That keeps a single bad name isolated to processOneRandomFile's catch (marked | ||
| // failed and retry-counted) instead of escaping as a RuntimeException that aborts the whole | ||
| // in-progress sweep for the cycle. | ||
| throw new IOException("Cannot extract timestamp from replication log file name: " + path, e); |
Contributor
Author
There was a problem hiding this comment.
.plog whose leading segment isn't numeric can't reach processFile on the replay side through the normal file lifecycle, so there's no file that used to be replayed and is now marked failed.
The catch is added as defensive check. It can only fire for a name introduced by on-disk corruption or manual placement. For that case, isolating the file as an ordinary per-file failure (marked failed, retry-counted, sweep continues) instead of replaying that file.
Comment on lines
+69
to
+72
| * Increments the counter for rounds whose new-file processing time exceeded the round time. This | ||
| * counter lives on the shared discovery base, so it is emitted by both the forwarder-side and | ||
| * replay-side discovery sources; a rising rate signals that that side's new-file processing is | ||
| * falling behind the cadence at which new rounds become eligible. |
Contributor
Author
There was a problem hiding this comment.
It's intentional as both replay and forwarder. Updated the PR description.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds six per-HA-group performance metrics for HA replication — new observability surface, no behavioral change.
Replay-side metrics (recorded by the standby replay discovery source):
Shared discovery/tracker counters — recorded in the shared base classes, so they are emitted by both the forwarder-side and replay-side metric sources (not replay-only)