PHOENIX-7973 HA client can adopt a stale (lower-version) ClusterRoleRecord when one endpoint lags the peer - #2589
Open
lokiore wants to merge 3 commits into
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>
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
1 participant
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.
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))