From 8be8b8e81f59a75bfe1f3acf7ddeb8ffd4c2e3e3 Mon Sep 17 00:00:00 2001 From: Lokesh Khurana Date: Mon, 27 Jul 2026 15:49:39 -0700 Subject: [PATCH 1/2] PHOENIX-7973 HA client can adopt a stale (lower-version) ClusterRoleRecord 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 --- .../phoenix/jdbc/HighAvailabilityGroup.java | 87 +++++++++++++------ .../jdbc/HighAvailabilityGroupTest.java | 49 +++++++++++ 2 files changed, 111 insertions(+), 25 deletions(-) diff --git a/phoenix-core-client/src/main/java/org/apache/phoenix/jdbc/HighAvailabilityGroup.java b/phoenix-core-client/src/main/java/org/apache/phoenix/jdbc/HighAvailabilityGroup.java index 017f996a319..1b74dc2cfaa 100644 --- a/phoenix-core-client/src/main/java/org/apache/phoenix/jdbc/HighAvailabilityGroup.java +++ b/phoenix-core-client/src/main/java/org/apache/phoenix/jdbc/HighAvailabilityGroup.java @@ -979,8 +979,17 @@ private static void throwMalFormedConnectionUrlException(String message) throws } /** - * Method to get ClusterRoleRecord from RegionServer Endpoints from either of the clusters. - * @return ClusterRoleRecord from the first available cluster + * Method to get ClusterRoleRecord from RegionServer Endpoints of both clusters. + *

+ * CRR version propagation across the RegionServers of the two clusters 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. To avoid the client adopting a staler + * or less usable record than one a peer already advertises, this always fetches the CRR from + * both endpoints and reconciles them via {@link #reconcileClusterRoleRecords}. If the + * peer (cluster 2) endpoint is unreachable, cluster 1's record is used as-is. The reconciliation + * subsumes the previous UNKNOWN-only handling. CRR is fetched only on connect/refresh (and cached + * in {@link #roleRecord}), not per query, so the extra endpoint RPC is not on a hot path. + * @return the reconciled ClusterRoleRecord * @throws SQLException if there is an error getting the ClusterRoleRecord */ private ClusterRoleRecord getClusterRoleRecordFromEndpoint() throws SQLException { @@ -992,31 +1001,22 @@ private ClusterRoleRecord getClusterRoleRecordFromEndpoint() throws SQLException // Get the CRR via RSEndpoint for cluster 1 ClusterRoleRecord roleRecord = GetClusterRoleRecordUtil.fetchClusterRoleRecord(info.getUrl1(), info.getUrl2(), info.getUrl1(), info.getName(), this, pollerInterval, properties); - // If we have unknown role for any cluster then try getting CRR from cluster 2 endpoint and if - // we get unknown role from there as well then CRR with higher adminVersion wins. - if (roleRecord.hasUnknownRole()) { - ClusterRoleRecord roleRecordFromPR; - try { - roleRecordFromPR = GetClusterRoleRecordUtil.fetchClusterRoleRecord(info.getUrl1(), - info.getUrl2(), info.getUrl2(), info.getName(), this, pollerInterval, properties); - } catch (Exception e) { - // As we were able to get CRR from cluster 1 but cluster 2 threw exception then just - // return - // CRR from cluster 1 and consume this exception - LOG.warn("Role Record from cluster {} has Unknown Role but cluster {} threw exception, " - + "returning {} as CRR", info.getUrl1(), info.getUrl2(), roleRecord.toPrettyString()); - return roleRecord; - } - if (roleRecordFromPR.hasUnknownRole()) { - return roleRecord.getVersion() > roleRecordFromPR.getVersion() - ? roleRecord - : roleRecordFromPR; - } else { - return roleRecordFromPR; - } - } else { + // Always consult cluster 2 as well and keep the more authoritative record (see method + // javadoc): a non-UNKNOWN record is preferred over an UNKNOWN one, and among records in the + // same category the higher admin version wins. If cluster 2 is unreachable, fall back to the + // cluster 1 record we already have and consume the exception. + ClusterRoleRecord roleRecordFromPR; + try { + roleRecordFromPR = GetClusterRoleRecordUtil.fetchClusterRoleRecord(info.getUrl1(), + info.getUrl2(), info.getUrl2(), info.getName(), this, pollerInterval, properties); + } catch (Exception e) { + LOG.warn( + "Fetched CRR {} from cluster {} but cluster {} endpoint threw an exception; " + + "returning cluster {} CRR without peer reconciliation", + roleRecord.toPrettyString(), info.getUrl1(), info.getUrl2(), info.getUrl1(), e); return roleRecord; } + return reconcileClusterRoleRecords(roleRecord, roleRecordFromPR); } catch (Exception e) { // If we get CRR Not Found on cluster 1, we should still try cluster 2, maybe // haGroupStoreClient @@ -1240,4 +1240,41 @@ static boolean shouldCountFailover(boolean transitionSucceeded, ClusterRoleRecor return transitionSucceeded && !oldRecord.getActiveUrl().equals(newRecord.getActiveUrl()) && newRecord.getActiveUrl().isPresent(); } + + /** + * Reconcile the two ClusterRoleRecords fetched from the cluster 1 and cluster 2 RegionServer + * endpoints and return the more authoritative one. CRR version propagation across RegionServers + * is not synchronized, so the two endpoints may disagree during startup or an in-flight + * transition; picking the more authoritative record prevents the client from adopting a staler or + * less usable view than one a peer already advertises. The order of preference is: + *

    + *
  1. A record without an UNKNOWN role beats a record with an UNKNOWN role. An UNKNOWN role means + * that endpoint could not resolve the cluster roles (e.g. a ZooKeeper problem) and is not usable + * for routing, so it must never win merely on version.
    2. + *
  2. Among records in the same UNKNOWN category, the higher admin {@code version} wins (the + * admin version only advances on an operator-driven CRR change, so higher is strictly + * fresher).
    3. + *
  3. On a tie (same category and version) the peer ({@code recordFromCluster2}) record is + * returned; both are equivalent for routing so the choice is arbitrary.
    4. + *
+ * This subsumes the previous UNKNOWN-only handling and is a strict superset of it: when cluster 1 + * is non-UNKNOWN and equal-or-newer it is still returned, and the UNKNOWN/UNKNOWN case still + * resolves to the higher version. Pure function of its inputs (no global state, no clock) so it + * is straightforward to unit-test. Package-private rather than private so + * {@code HighAvailabilityGroupTest} can call it directly. Neither argument may be {@code null}. + * @param recordFromCluster1 CRR fetched from the cluster 1 endpoint ({@code info.getUrl1()}) + * @param recordFromCluster2 CRR fetched from the cluster 2 endpoint ({@code info.getUrl2()}) + * @return the more authoritative of the two records + */ + static ClusterRoleRecord reconcileClusterRoleRecords(ClusterRoleRecord recordFromCluster1, + ClusterRoleRecord recordFromCluster2) { + // Prefer a usable (non-UNKNOWN) record over an UNKNOWN one regardless of version. + if (recordFromCluster1.hasUnknownRole() != recordFromCluster2.hasUnknownRole()) { + return recordFromCluster1.hasUnknownRole() ? recordFromCluster2 : recordFromCluster1; + } + // Same category: keep the higher admin version. Ties fall through to the peer record. + return recordFromCluster1.getVersion() > recordFromCluster2.getVersion() + ? recordFromCluster1 + : recordFromCluster2; + } } diff --git a/phoenix-core/src/test/java/org/apache/phoenix/jdbc/HighAvailabilityGroupTest.java b/phoenix-core/src/test/java/org/apache/phoenix/jdbc/HighAvailabilityGroupTest.java index cc58727838d..7a74ea39b72 100644 --- a/phoenix-core/src/test/java/org/apache/phoenix/jdbc/HighAvailabilityGroupTest.java +++ b/phoenix-core/src/test/java/org/apache/phoenix/jdbc/HighAvailabilityGroupTest.java @@ -185,4 +185,53 @@ public void testShouldCountFailoverGate() { + "should count as a failover", HighAvailabilityGroup.shouldCountFailover(true, bothStandby, aStandbyBActive)); } + + /** + * Verifies the two-endpoint reconciliation performed by + * {@link HighAvailabilityGroup#reconcileClusterRoleRecords} — exercised directly via the + * package-private helper rather than by driving two mini-cluster endpoints. The client fetches + * the CRR from both cluster endpoints and must keep the more authoritative record so that a + * momentarily stale endpoint (lower admin version) or an endpoint that cannot resolve roles + * (UNKNOWN) never causes the client to adopt a staler / less usable view than its peer. This pins + * down: (a) higher version wins when both are usable — the regression guard for the stale-revert + * bug; (b) it is order-independent; (c) a non-UNKNOWN record beats an UNKNOWN one regardless of + * version, in both argument orders; and (d) UNKNOWN vs UNKNOWN still resolves to the higher + * version (preserving the previous behavior). + */ + @Test + public void testReconcileClusterRoleRecords() { + String haGroupName = "testReconcileClusterRoleRecords"; + String url1 = "host1\\:60010"; + String url2 = "host2\\:60010"; + + ClusterRoleRecord v9 = new ClusterRoleRecord(haGroupName, HighAvailabilityPolicy.FAILOVER, url1, + ClusterRole.ACTIVE, url2, ClusterRole.STANDBY, 9L); + ClusterRoleRecord v10 = new ClusterRoleRecord(haGroupName, HighAvailabilityPolicy.FAILOVER, + url1, ClusterRole.STANDBY, url2, ClusterRole.ACTIVE, 10L); + ClusterRoleRecord unknownV11 = new ClusterRoleRecord(haGroupName, + HighAvailabilityPolicy.FAILOVER, url1, ClusterRole.UNKNOWN, url2, ClusterRole.STANDBY, 11L); + ClusterRoleRecord unknownV12 = new ClusterRoleRecord(haGroupName, + HighAvailabilityPolicy.FAILOVER, url1, ClusterRole.UNKNOWN, url2, ClusterRole.UNKNOWN, 12L); + + // (a) Both usable (non-UNKNOWN): the higher admin version wins. This is the core fix — a + // stale endpoint reporting v9 must not override the peer's v10. Regression guard for the + // silent stale-revert. + assertTrue("Higher version must win when cluster 1 lags the peer", + v10 == HighAvailabilityGroup.reconcileClusterRoleRecords(v9, v10)); + + // (b) Order-independent: same result regardless of which endpoint is passed first. + assertTrue("Higher version must win regardless of argument order", + v10 == HighAvailabilityGroup.reconcileClusterRoleRecords(v10, v9)); + + // (c) A non-UNKNOWN record beats an UNKNOWN one even when the UNKNOWN record has a higher + // version — an UNKNOWN role is not usable for routing. Assert both argument orders. + assertTrue("Non-UNKNOWN must beat UNKNOWN even with a lower version (cluster 1 usable)", + v9 == HighAvailabilityGroup.reconcileClusterRoleRecords(v9, unknownV11)); + assertTrue("Non-UNKNOWN must beat UNKNOWN even with a lower version (cluster 2 usable)", + v9 == HighAvailabilityGroup.reconcileClusterRoleRecords(unknownV11, v9)); + + // (d) UNKNOWN vs UNKNOWN → higher version wins (preserves the prior behavior). + assertTrue("Among two UNKNOWN records the higher version wins", + unknownV12 == HighAvailabilityGroup.reconcileClusterRoleRecords(unknownV11, unknownV12)); + } } From fa2b265597f9f48bb3eb3ca6491950b1f330abfe Mon Sep 17 00:00:00 2001 From: Lokesh Khurana Date: Wed, 29 Jul 2026 09:44:39 -0700 Subject: [PATCH 2/2] PHOENIX-7973 Guard the CRR refresh path against rolling back to a lower 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 --- .../phoenix/jdbc/HighAvailabilityGroup.java | 44 +++++++++++++++++++ .../jdbc/HighAvailabilityGroupTest.java | 43 ++++++++++++++++++ 2 files changed, 87 insertions(+) diff --git a/phoenix-core-client/src/main/java/org/apache/phoenix/jdbc/HighAvailabilityGroup.java b/phoenix-core-client/src/main/java/org/apache/phoenix/jdbc/HighAvailabilityGroup.java index 1b74dc2cfaa..b294f14fa12 100644 --- a/phoenix-core-client/src/main/java/org/apache/phoenix/jdbc/HighAvailabilityGroup.java +++ b/phoenix-core-client/src/main/java/org/apache/phoenix/jdbc/HighAvailabilityGroup.java @@ -1106,6 +1106,22 @@ public boolean refreshClusterRoleRecord(boolean forceRefresh) throws SQLExceptio return true; } + // Do not roll back to a lower-version record. CRR propagation across a cluster's + // RegionServers is eventually consistent, so a lagging endpoint can momentarily serve an + // older admin version than the one already applied on this client. Since the endpoint is + // picked at (effectively) random on each fetch, adopting such a record would revert the + // client to a stale cluster-role view. The apply-or-reject decision is factored into the + // package-private static {@link #shouldApplyRefreshedRecord} so it can be unit-tested + // directly without driving a full mini-cluster refresh. + if (!shouldApplyRefreshedRecord(roleRecord, newRoleRecord)) { + LOG.warn( + "Fetched role record {} is older (V{}) than the current record (V{}) for HA group {};" + + " keeping the current record and not rolling back", + newRoleRecord, newRoleRecord.getVersion(), roleRecord.getVersion(), info); + lastClusterRoleRecordRefreshTime = System.currentTimeMillis(); + return true; + } + final ClusterRoleRecord oldRecord = roleRecord; state = State.IN_TRANSITION; LOG.info("HA group {} is in {} to set V{} record", info, state, newRoleRecord.getVersion()); @@ -1277,4 +1293,32 @@ static ClusterRoleRecord reconcileClusterRoleRecords(ClusterRoleRecord recordFro ? recordFromCluster1 : recordFromCluster2; } + + /** + * Decides whether a freshly fetched {@link ClusterRoleRecord} should replace the currently + * applied one on the refresh path. Returns {@code false} only when the current record is strictly + * newer (higher admin {@code version}) than the fetched one, i.e. the fetch would roll the client + * back to a stale view. This guards against eventual-consistency lag: CRR propagation across a + * cluster's RegionServers is not synchronized, so a lagging endpoint (the client picks one at + * effectively random per fetch) can momentarily serve an older admin version than the client has + * already applied. + *

+ * An equal version returns {@code true} (apply) by design. The admin {@code version} + * only advances on an operator-driven CRR change; an autonomous state-machine transition changes + * the cluster roles while keeping the same admin version, so a same-version record with different + * roles is a legitimate update that must still take effect. Only a strictly lower version is + * rejected — equivalently, this returns {@code !current.isNewerThan(fetched)}. + *

+ * Callers guarantee {@code current} and {@code fetched} are non-null and share the same HA group + * info (checked upstream), and that they are not {@code equals()} (the no-op case is handled + * before this). Pure function of its inputs (no global state, no clock) so the guard is + * unit-testable without driving a full mini-cluster refresh. Package-private rather than private + * so {@code HighAvailabilityGroupTest} can call it directly. + * @param current the currently applied {@link ClusterRoleRecord} (must be non-null) + * @param fetched the candidate record freshly fetched from the endpoints + * @return {@code true} to apply {@code fetched}; {@code false} to keep {@code current} + */ + static boolean shouldApplyRefreshedRecord(ClusterRoleRecord current, ClusterRoleRecord fetched) { + return !current.isNewerThan(fetched); + } } diff --git a/phoenix-core/src/test/java/org/apache/phoenix/jdbc/HighAvailabilityGroupTest.java b/phoenix-core/src/test/java/org/apache/phoenix/jdbc/HighAvailabilityGroupTest.java index 7a74ea39b72..99d9533e648 100644 --- a/phoenix-core/src/test/java/org/apache/phoenix/jdbc/HighAvailabilityGroupTest.java +++ b/phoenix-core/src/test/java/org/apache/phoenix/jdbc/HighAvailabilityGroupTest.java @@ -234,4 +234,47 @@ public void testReconcileClusterRoleRecords() { assertTrue("Among two UNKNOWN records the higher version wins", unknownV12 == HighAvailabilityGroup.reconcileClusterRoleRecords(unknownV11, unknownV12)); } + + /** + * Verifies the refresh-path apply/reject decision performed by + * {@link HighAvailabilityGroup#shouldApplyRefreshedRecord} — exercised directly via the + * package-private helper rather than by driving a full mini-cluster refresh. CRR propagation + * across a cluster's RegionServers is eventually consistent and the client picks an endpoint at + * effectively random per fetch, so a lagging endpoint can momentarily return a lower admin + * version than the client has already applied; the client must not roll back to it. This pins + * down: (a) a strictly lower-version record is rejected (the regression guard for the + * stale-revert bug); (b) a strictly higher-version record is applied; and — the subtle case — (c) + * a SAME-version record with different roles is applied, because an autonomous state-machine + * transition changes roles while keeping the same admin version (only an operator/admin action + * bumps the version), so a strict {@code >} guard here would wrongly strand failover. + */ + @Test + public void testShouldApplyRefreshedRecord() { + String haGroupName = "testShouldApplyRefreshedRecord"; + String url1 = "host1\\:60010"; + String url2 = "host2\\:60010"; + + ClusterRoleRecord v9 = new ClusterRoleRecord(haGroupName, HighAvailabilityPolicy.FAILOVER, url1, + ClusterRole.ACTIVE, url2, ClusterRole.STANDBY, 9L); + ClusterRoleRecord v10 = new ClusterRoleRecord(haGroupName, HighAvailabilityPolicy.FAILOVER, + url1, ClusterRole.STANDBY, url2, ClusterRole.ACTIVE, 10L); + // Same version as v10 but different roles — models an autonomous state-machine transition, + // which changes roles while keeping the admin version unchanged. + ClusterRoleRecord v10RolesChanged = new ClusterRoleRecord(haGroupName, + HighAvailabilityPolicy.FAILOVER, url1, ClusterRole.STANDBY, url2, ClusterRole.STANDBY, 10L); + + // (a) Current is newer (v10) than the fetched record (v9): reject, do not roll back. This is + // the core fix — a lagging endpoint serving v9 must not override the applied v10. + assertFalse("Must not roll back from the applied v10 to a stale fetched v9", + HighAvailabilityGroup.shouldApplyRefreshedRecord(v10, v9)); + + // (b) Fetched record is newer (v10) than the current (v9): apply it. + assertTrue("Must apply a strictly newer fetched record", + HighAvailabilityGroup.shouldApplyRefreshedRecord(v9, v10)); + + // (c) SAME version, different roles (autonomous transition): must still apply. A strict '>' + // guard would reject this and strand the client on a stale role view — regression guard. + assertTrue("Must apply a same-version record with changed roles (autonomous transition)", + HighAvailabilityGroup.shouldApplyRefreshedRecord(v10, v10RolesChanged)); + } }