PHOENIX-7973 HA client can adopt a stale (lower-version) ClusterRoleRecord when one endpoint lags the peer

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.
+   * <p>
+   * 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
+   * <em>both</em> 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:
+   * <ol>
+   * <li>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.</li>
+   * <li>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).</li>
+   * <li>On a tie (same category and version) the peer ({@code recordFromCluster2}) record is
+   * returned; both are equivalent for routing so the choice is arbitrary.</li>
+   * </ol>
+   * 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;
+  }
+
+  /**
+   * 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.
+   * <p>
+   * An <em>equal</em> 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)}.
+   * <p>
+   * 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);
+  }
 }

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));
+  }
 }