PHOENIX-7973 HA client can adopt a stale (lower-version) ClusterRoleRecord when one endpoint lags the peer - #2589
Conversation
…ecord when one endpoint lags the peer getClusterRoleRecordFromEndpoint() queried cluster 1 first and returned it immediately whenever it had no UNKNOWN role, without consulting cluster 2. CRR version propagation across RegionServers is not synchronized, so at startup or during an in-flight admin/failover transition one endpoint can momentarily serve a lower admin version (or an UNKNOWN role) than its peer. In that window the client adopted the staler, lower-version record and silently reverted to an older cluster-role view. The refresh path guards only with ClusterRoleRecord.equals() (which ignores version) and never called the existing isNewerThan() helper, so nothing detected the downgrade. Fix: always fetch the CRR from both cluster endpoints and reconcile via a new package-private static reconcileClusterRoleRecords(): prefer a record without an UNKNOWN role (a known-role record is usable for routing; an UNKNOWN one is not), and within the same category prefer the higher admin version. This is a strict superset of the previous UNKNOWN-only handling and guarantees the client never adopts a CRR older than one a peer already advertises. If the peer endpoint is unreachable, cluster 1's record is used as-is. Adds one endpoint RPC to the CRR refresh path only; CRR is fetched on connect/refresh (cached), not per query, so no meaningful perf impact. Client-side only; no API or wire-format change. Unit-tested via HighAvailabilityGroupTest#testReconcileClusterRoleRecords (higher-version-wins regression guard, order-independence, non-UNKNOWN beats UNKNOWN in both orders, UNKNOWN-vs-UNKNOWN higher-version). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…er version The refresh path applied any non-equals() ClusterRoleRecord fetched from the endpoints, with no version comparison. Because CRR propagation across a cluster's RegionServers is eventually consistent and the client picks an endpoint at (effectively) random per fetch, a lagging endpoint can momentarily serve an older admin version than the client has already applied, silently reverting the client to a stale cluster-role view. Add a shouldApplyRefreshedRecord(current, fetched) guard that keeps the current record when it is strictly newer than the fetched one (equivalently, !current.isNewerThan(fetched)). An equal admin version is intentionally still applied: the admin version only advances on an operator-driven change, so an autonomous state-machine transition changes the cluster roles while keeping the same version, and that legitimate same-version role change must still take effect. Only a strictly lower version is rejected, so a strict '>' guard is deliberately avoided. The decision is factored into a package-private static helper (mirroring shouldCountFailover / reconcileClusterRoleRecords) and unit-tested in HighAvailabilityGroupTest#testShouldApplyRefreshedRecord: (a) reject a strictly lower version, (b) apply a strictly higher version, (c) apply a same-version record with changed roles. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…lpers Condense the method Javadocs, inline comments, and test Javadocs added for the two-endpoint reconciliation and refresh guard down to the non-obvious contract (UNKNOWN-not-usable-for-routing, higher-version-wins, same-version still applied for autonomous transitions, package-private-for-test). No behavior change; comments only. Generated-by: Claude Code (Opus 4.8) Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Review — PHOENIX-7973: Guard HA client against stale (lower-version) ClusterRoleRecord
Well-motivated and tightly scoped. Two pure, package-private helpers carry the decision logic and are directly unit-tested. No import/style violations (no wildcards, no unshaded Guava, SLF4J throughout, no new imports). The perf claim is accurate — the now-unconditional second RPC is on the refresh path only (gated behind shouldRefreshRoleRecord()), not the query hot path.
The core logic is sound, but there is one real correctness gap at the equal-version boundary plus a few medium error-handling/observability items.
Important
1. Equal-version tie-break can adopt a stale peer record and trigger an erroneous transition — HighAvailabilityGroup.java:1261-1264 + :1101
The design premise (test v10RolesChanged, Javadoc at :1270-1276) is that an autonomous transition changes roles while keeping the same admin version. So two endpoints can legitimately report version N with different roles when one lags. Then:
- reconcile sees equal versions, both non-UNKNOWN → the tie falls through to
recordFromCluster2(:1261-1264), which may be the stale peer. - In refresh:
equals()is false (roles differ), andshouldApplyRefreshedRecord = !current.isNewerThan(fetched) = !(N > N) = true→ the stale record is applied and a role transition fires.
The "do not roll back" guard defends only against strictly lower versions, so equal-version divergence re-opens the exact stale-adoption window the guard leaves open by design, and can cause a failover flap. This is also a behavior change from the old code (which returned cluster 1 immediately): the outcome now depends arbitrarily on which endpoint lags. Consider having the tie prefer the record whose roles match the currently-applied one, or not transitioning on an equal-version divergence. Self-corrects once the peer catches up, but the choice should not be arbitrary.
Suggested test: assertSame on two same-version different-role records, plus a refresh-level assertion that an equal-version divergent record from a lagging peer does not trigger a spurious transition.
Medium
2. Reconcile decision is unobservable — :1250-1261, invoked at :1010
The method makes a consequential trust decision (which endpoint to believe when they diverge) with no logging. Debugging "why does the client keep routing to the stale cluster" leaves no trace of the divergent pair or which record won. The first-load path (roleRecord == null, :1073) adopts whatever reconcile returns with no version guard and no log. Suggest an INFO/DEBUG line at the call site when the two records differ, naming both versions/roles and the choice.
3. catch (Exception) in the cluster-2 fetch is too broad and drops interrupt status — :1003-1009
The fallback (return cluster 1 when the peer is unreachable) and its WARN log are correct — good level, both URLs, the record, and e passed for the stack trace. But catching Exception also swallows InterruptedException without restoring the interrupt flag (contrast :1130-1133, which re-interrupts) and downgrades unchecked bugs (RejectedExecutionException, NPE) to a benign "peer unreachable" WARN. Suggest narrowing to SQLException and restoring interrupt status if the cause is an interruption.
Minor
4. Non-UNKNOWN preference can transiently mask a genuinely newer UNKNOWN state — :1257-1259. UNKNOWN is a valid persisted role; if an admin bump legitimately drives a cluster to UNKNOWN at version N+1 while an endpoint lags at non-UNKNOWN N, reconcile returns the stale N. Guarded on the refresh path, but not on first-load. The Javadoc's "never wins on version alone" (:1247-1249) slightly overstates that UNKNOWN is always non-authoritative.
5. Javadoc/code drift — getClusterRoleRecordFromEndpoint says @return the reconciled ClusterRoleRecord (:986), but the outer catch fallbacks at :1021 and :1029 return un-reconciled records. The doc only mentions the "peer unreachable → cluster 1" case.
6. Test polish — prefer assertSame(expected, actual) over assertTrue("...", a == b) (HighAvailabilityGroupTest.java:207-219); identical behavior, far better failure diagnostics. The == identity intent itself is correct and stronger than equals here.
Test coverage gaps
Helper logic is well covered. Untested: reconciliation wiring in getClusterRoleRecordFromEndpoint (an argument swap would pass every helper test); the new refresh guard branch (:1100-1108) returning true, updating the refresh time, and not transitioning — the exact regression this PR fixes; the version-tie / same-version-different-roles reconcile case; and the cluster-2-unreachable fallback.
Strengths
- Clean separation of pure decision logic into testable helpers.
- Good WARN logging on the two new failure/rejection paths.
shouldApplyRefreshedRecorddeliberately uses>=semantics (via!isNewerThan); the test explicitly guards against a>-mutation with the same-version role-change case.- Accurate perf reasoning; refresh-path-only cost.
Merge-blocking in my view: only finding 1. The rest are follow-ups.
There was a problem hiding this comment.
Pull request overview
Hardens HA client CRR reconciliation to prevent stale version rollback.
Changes:
- Reconciles records from both endpoints by role usability and version.
- Rejects lower-version records during refresh.
- Adds unit tests for reconciliation and refresh guards.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
HighAvailabilityGroup.java |
Adds dual-endpoint reconciliation and rollback prevention. |
HighAvailabilityGroupTest.java |
Tests version and role reconciliation behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| roleRecordFromPR = GetClusterRoleRecordUtil.fetchClusterRoleRecord(info.getUrl1(), | ||
| info.getUrl2(), info.getUrl2(), info.getName(), this, pollerInterval, properties); |
…ist poller scheduling out of the raw fetch Two related hardening changes to the two-endpoint CRR reconciliation path: - Reconcile now takes the currently-applied record so an equal-version divergence (both endpoints at the same admin version but with different roles, one endpoint lagging) defers to the applied record instead of arbitrarily adopting the peer and flapping. The defer is scoped to current's version so a genuine version advance is never suppressed. A strictly-newer UNKNOWN-tagged record that still names an active cluster now wins over a stale fully-known record (mid-transition state advance); a newer UNKNOWN record with no active role stays masked and the non-active poller resolves it. - Poller scheduling is hoisted out of the raw endpoint fetch. getClusterRoleRecord is now a pure read; the non-active poller is scheduled at most once, after reconciliation, via maybeSchedulePoller on the resolved record. Scheduling off a single raw fetch churned a poller for an ACTIVE-plus-stale-peer state and widened a pollerLock/write-lock AB-BA inversion into a reachable deadlock; the winner tick now refreshes OUTSIDE pollerLock. - The cluster-2 fetch failure path catches broadly (an unchecked exception from the pre-RPC connect path is still a reachability failure) and restores the thread interrupt status when the failure wrapped an interruption. Unit-tested in HighAvailabilityGroupTest: reconcile UNKNOWN/version precedence incl. the strict newer-active-unknown boundary, and equal-version divergence deferral/convergence. Generated-by: Claude Code (Opus 4.8) Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Review — dual-endpoint CRR reconcile + refresh guardVerdict: approve with minor follow-ups. Core logic is correct on the three highest-risk areas — the reconcile decision tree, the AB-BA deadlock fix, and poller-scheduling behavior. No correctness or concurrency defects found in the new code. Items below are a doc fix, diagnostics gaps, and test wiring. Verified solid
Important (fix before merge)
Concerns (diagnostics — new code)
Pre-existing (adjacent to the touched poller block — not introduced here; suggest a follow-up ticket)
Test coverageHelpers are well-covered — both arg orders, version ties, the strict-
Nothing here is a correctness blocker. Reviewed with assistance from Claude Code (Opus 4.8). |
ba153a5 to
6b24780
Compare
…ling, and endpoint/refresh test wiring Follow-up to the reviewer's minor items on the two-endpoint CRR path (no correctness/concurrency changes to the reconcile decision tree itself): - getClusterRoleRecordFromEndpoint restructured so reconciliation runs OUTSIDE both per-endpoint fetch try/catch blocks. A bug in reconcile now surfaces as itself rather than being caught by a fetch catch, misattributed as an endpoint failure, and silently degraded to a single-endpoint record. - The cluster-1 failure path now logs the cluster-1 exception (WARN) before falling back to cluster 2, so a cluster-1 failure (including an unchecked bug such as an NPE) no longer vanishes when cluster 2 succeeds. The interrupt status the fetch cleared is restored in a finally after the fallback fetch, so a stale flag cannot pre-empt the blocking work while callers still observe cancellation. - On both cluster-1 fallback paths, if cluster 2 also fails the cluster-1 failure is retained via addSuppressed instead of being dropped: the CRR-Not- Found path rethrows the original Not-Found (cluster-2 suppressed) so downstream fallback still triggers, and the non-Not-Found path rethrows cluster 2's exception with cluster 1's attached as suppressed, so the single propagating exception carries both root causes. - Corrected the reconcile carve-out comment (and its mirror in the test): a newer-UNKNOWN-with-no-active record stays masked behind the usable record and recovery comes from the next scheduled refresh (the poller runs only if the usable record is itself non-active). Tests added in HighAvailabilityGroupTest: - testIsCausedByInterrupt: both interrupt marker types, wrapped, non-interrupt, null, the depth-16 bound (within and beyond), and a cyclic cause chain. - testRefreshDoesNotRollBackToOlderRecord: wires the refresh no-rollback branch end to end (keeps the applied record, stays READY, no failover count, returns true). - testGetClusterRoleRecordFromEndpointWiring: pins url1->cluster1 / url2->cluster2 and that the applied record is threaded as current on the equal-version defer. - Equal-version divergence with the applied record at a lower version than the endpoints now covered (the non-first-load fall-through). - Endpoint fallback/rethrow branches: NOT_FOUND-on-cluster-1 falls back to a cluster-2 record; both-fail on the NOT_FOUND path rethrows the original Not-Found with the cluster-2 failure suppressed (error code preserved); both-fail on the non-Not-Found path rethrows cluster 2's exception with cluster 1's suppressed; a cluster-2 failure with cluster 1 reachable degrades to the cluster-1 record; and an interrupt-wrapped cluster-1 failure restores the thread interrupt status on the fallback path. Small testability seams added: package-private @VisibleForTesting on getClusterRoleRecordFromEndpoint, isCausedByInterrupt, a fetchClusterRoleRecord per-endpoint seam, and getStateForTesting. Generated-by: Claude Code (Opus 4.8) Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
6b24780 to
6fe0dab
Compare
What changes were proposed in this pull request?
Hardens the HA client against adopting a stale, lower-version
ClusterRoleRecord(CRR) when one RegionServer endpoint lags its peer. Client-side only; no API/wire change. Base:PHOENIX-7562-feature-new.getClusterRoleRecordFromEndpoint): now fetches from both cluster endpoints and reconciles via new helperreconcileClusterRoleRecords(r1, r2)— non-UNKNOWNbeatsUNKNOWN; else higher adminversionwins; tie → peer. Previously returned cluster 1 immediately whenever it had noUNKNOWNrole.refreshClusterRoleRecord): new helpershouldApplyRefreshedRecord(current, fetched)rejects a strictly-lower version viaisNewerThan(). Equal version is still applied, so autonomous same-version role transitions still take effect.Why are the changes needed?
CRR admin-version propagation across a cluster's RegionServers is not synchronized and the client picks an endpoint per fetch, so during startup/transition one endpoint can briefly serve a lower version (or
UNKNOWN). The old refresh path guarded only withequals()(ignoresversion), letting a lagging endpoint silently revert the client to a stale view.Does this PR introduce any user-facing change?
No.
How was this patch tested?
New unit tests in
HighAvailabilityGroupTest(no mini-cluster):testReconcileClusterRoleRecordsandtestShouldApplyRefreshedRecord(reject lower, apply higher, apply same-version-with-changed-roles).spotless:checkclean. Adds one endpoint RPC on the refresh path only — not the query hot path.Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Opus 4.8 (1M context))