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.
2 participants
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))