+ * 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 @@ -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()); @@ -1240,4 +1256,69 @@ 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: + *
+ * 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 cc58727838d..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 @@ -185,4 +185,96 @@ 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)); + } + + /** + * 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)); + } }