Himanshu-g81 commented on code in PR #2604:
URL: https://github.com/apache/phoenix/pull/2604#discussion_r3830360469
##########
phoenix-core/src/it/java/org/apache/phoenix/jdbc/HAGroupStoreClientIT.java:
##########
@@ -1537,6 +1537,234 @@ public void
testSetHAGroupStatusIfNeededMultipleTransitions() throws Exception {
assertEquals(HAGroupStoreRecord.HAGroupState.STANDBY,
afterSecond.getHAGroupState());
}
+ /**
+ * A same-state write on the first attempt is an intentional refresh, not a
no-op: the periodic
+ * STORE_AND_FORWARD heartbeat re-writes ACTIVE_NOT_IN_SYNC to bump the
znode mtime so the
+ * standby's staleness check stays fresh (see
+ * StoreAndForwardModeImpl#startHAGroupStoreUpdateTask). It must proceed to
the CAS and bump the
+ * version. The no-op short-circuit applies only on the reconcile retry path
after a stale-version
+ * CAS loss (see the convergent-race test).
+ */
+ @Test
+ public void testSetHAGroupStatusIfNeededSameStateRefreshBumpsVersion()
throws Exception {
+ String haGroupName = testName.getMethodName();
+
+ HAGroupStoreRecord initialRecord = new HAGroupStoreRecord("v1.0",
haGroupName,
+ HAGroupStoreRecord.HAGroupState.ACTIVE_NOT_IN_SYNC, 0L,
+ HighAvailabilityPolicy.FAILOVER.toString(), this.peerZKUrl,
this.masterUrl,
+ this.peerMasterUrl, CLUSTERS.getHdfsUrl1(), CLUSTERS.getHdfsUrl2(), 0L);
+ createOrUpdateHAGroupStoreRecordOnZookeeper(haAdmin, haGroupName,
initialRecord);
+
+ HAGroupStoreClient haGroupStoreClient = HAGroupStoreClient
+ .getInstanceForZkUrl(CLUSTERS.getHBaseCluster1().getConfiguration(),
haGroupName, zkUrl);
+ Thread.sleep(ZK_CURATOR_EVENT_PROPAGATION_TIMEOUT_MS);
+ int versionBefore =
+
haAdmin.getHAGroupStoreRecordInZooKeeper(haGroupName).getRight().getVersion();
+
+ // Requested state == current state on the first attempt: this is the
heartbeat refresh; it must
+ // write and bump the version so the mtime advances.
+ assertEquals(0L, haGroupStoreClient
+
.setHAGroupStatusIfNeeded(HAGroupStoreRecord.HAGroupState.ACTIVE_NOT_IN_SYNC));
+
+ Thread.sleep(ZK_CURATOR_EVENT_PROPAGATION_TIMEOUT_MS);
+ Pair<HAGroupStoreRecord, Stat> after =
haAdmin.getHAGroupStoreRecordInZooKeeper(haGroupName);
+ assertEquals(HAGroupStoreRecord.HAGroupState.ACTIVE_NOT_IN_SYNC,
+ after.getLeft().getHAGroupState());
+ assertTrue("Same-state heartbeat refresh must bump the znode version",
+ after.getRight().getVersion() > versionBefore);
+ }
+
+ /**
+ * Convergent-race regression
(docs/HA_Status_CAS_Stale_Cache_BadVersion.md): a peer RS advances
Review Comment:
This doc reference need to be removed?
##########
phoenix-core/src/it/java/org/apache/phoenix/jdbc/HAGroupStoreClientIT.java:
##########
@@ -1537,6 +1537,234 @@ public void
testSetHAGroupStatusIfNeededMultipleTransitions() throws Exception {
assertEquals(HAGroupStoreRecord.HAGroupState.STANDBY,
afterSecond.getHAGroupState());
}
+ /**
+ * A same-state write on the first attempt is an intentional refresh, not a
no-op: the periodic
+ * STORE_AND_FORWARD heartbeat re-writes ACTIVE_NOT_IN_SYNC to bump the
znode mtime so the
+ * standby's staleness check stays fresh (see
+ * StoreAndForwardModeImpl#startHAGroupStoreUpdateTask). It must proceed to
the CAS and bump the
+ * version. The no-op short-circuit applies only on the reconcile retry path
after a stale-version
+ * CAS loss (see the convergent-race test).
+ */
+ @Test
+ public void testSetHAGroupStatusIfNeededSameStateRefreshBumpsVersion()
throws Exception {
+ String haGroupName = testName.getMethodName();
+
+ HAGroupStoreRecord initialRecord = new HAGroupStoreRecord("v1.0",
haGroupName,
+ HAGroupStoreRecord.HAGroupState.ACTIVE_NOT_IN_SYNC, 0L,
+ HighAvailabilityPolicy.FAILOVER.toString(), this.peerZKUrl,
this.masterUrl,
+ this.peerMasterUrl, CLUSTERS.getHdfsUrl1(), CLUSTERS.getHdfsUrl2(), 0L);
+ createOrUpdateHAGroupStoreRecordOnZookeeper(haAdmin, haGroupName,
initialRecord);
+
+ HAGroupStoreClient haGroupStoreClient = HAGroupStoreClient
+ .getInstanceForZkUrl(CLUSTERS.getHBaseCluster1().getConfiguration(),
haGroupName, zkUrl);
+ Thread.sleep(ZK_CURATOR_EVENT_PROPAGATION_TIMEOUT_MS);
+ int versionBefore =
+
haAdmin.getHAGroupStoreRecordInZooKeeper(haGroupName).getRight().getVersion();
+
+ // Requested state == current state on the first attempt: this is the
heartbeat refresh; it must
+ // write and bump the version so the mtime advances.
+ assertEquals(0L, haGroupStoreClient
+
.setHAGroupStatusIfNeeded(HAGroupStoreRecord.HAGroupState.ACTIVE_NOT_IN_SYNC));
+
+ Thread.sleep(ZK_CURATOR_EVENT_PROPAGATION_TIMEOUT_MS);
+ Pair<HAGroupStoreRecord, Stat> after =
haAdmin.getHAGroupStoreRecordInZooKeeper(haGroupName);
+ assertEquals(HAGroupStoreRecord.HAGroupState.ACTIVE_NOT_IN_SYNC,
+ after.getLeft().getHAGroupState());
+ assertTrue("Same-state heartbeat refresh must bump the znode version",
+ after.getRight().getVersion() > versionBefore);
+ }
+
+ /**
+ * Convergent-race regression
(docs/HA_Status_CAS_Stale_Cache_BadVersion.md): a peer RS advances
+ * the shared record to ACTIVE_NOT_IN_SYNC while this client still holds the
pre-bump cached
+ * version. The client's own transition to the same target must reconcile
(stale CAS -> re-read
+ * fresh -> observe target already met -> no-op success) rather than
aborting with
+ * StaleHAGroupStoreRecordVersionException. The external advance happens
immediately before the
+ * client call so the local cache is still at the stale AIS version and the
stale-CAS path is
+ * exercised. Version is asserted >= the winner's: if the watch had already
caught the cache up,
+ * attempt 1 would instead do a legitimate same-state heartbeat refresh (one
extra bump) — also
+ * correct; the invariant this test pins is "no abort, converges to
ACTIVE_NOT_IN_SYNC".
+ */
+ @Test
+ public void testSetHAGroupStatusIfNeededConvergentRaceReconciles() throws
Exception {
+ String haGroupName = testName.getMethodName();
+
+ HAGroupStoreRecord initialRecord =
+ new HAGroupStoreRecord("v1.0", haGroupName,
HAGroupStoreRecord.HAGroupState.ACTIVE_IN_SYNC,
+ 0L, HighAvailabilityPolicy.FAILOVER.toString(), this.peerZKUrl,
this.masterUrl,
+ this.peerMasterUrl, CLUSTERS.getHdfsUrl1(), CLUSTERS.getHdfsUrl2(),
0L);
+ createOrUpdateHAGroupStoreRecordOnZookeeper(haAdmin, haGroupName,
initialRecord);
+
+ HAGroupStoreClient haGroupStoreClient = HAGroupStoreClient
+ .getInstanceForZkUrl(CLUSTERS.getHBaseCluster1().getConfiguration(),
haGroupName, zkUrl);
+ Thread.sleep(ZK_CURATOR_EVENT_PROPAGATION_TIMEOUT_MS);
+ assertEquals(HAGroupStoreRecord.HAGroupState.ACTIVE_IN_SYNC,
+ haGroupStoreClient.getHAGroupStoreRecord().getHAGroupState());
+
+ // A peer RS wins the CAS: advance the shared znode to ACTIVE_NOT_IN_SYNC
out from under this
+ // client's cache, then immediately drive this client to the same target
before its watch fires.
+ Pair<HAGroupStoreRecord, Stat> current =
haAdmin.getHAGroupStoreRecordInZooKeeper(haGroupName);
+ haAdmin.updateHAGroupStoreRecordInZooKeeper(haGroupName,
+
current.getLeft().withHAGroupState(HAGroupStoreRecord.HAGroupState.ACTIVE_NOT_IN_SYNC),
+ current.getRight().getVersion());
+ int winnerVersion =
+
haAdmin.getHAGroupStoreRecordInZooKeeper(haGroupName).getRight().getVersion();
+
+ // Must not throw StaleHAGroupStoreRecordVersionException; converges as a
no-op success.
+ assertEquals(0L, haGroupStoreClient
+
.setHAGroupStatusIfNeeded(HAGroupStoreRecord.HAGroupState.ACTIVE_NOT_IN_SYNC));
+
+ Thread.sleep(ZK_CURATOR_EVENT_PROPAGATION_TIMEOUT_MS);
+ Pair<HAGroupStoreRecord, Stat> after =
haAdmin.getHAGroupStoreRecordInZooKeeper(haGroupName);
+ assertEquals(HAGroupStoreRecord.HAGroupState.ACTIVE_NOT_IN_SYNC,
+ after.getLeft().getHAGroupState());
+ assertTrue(
+ "Loser must converge without clobbering: version >= winner's (equal on
the stale-CAS "
+ + "reconcile path; winner+1 if the watch caught the cache up first and
attempt 1 did a "
+ + "legitimate same-state refresh)",
+ after.getRight().getVersion() >= winnerVersion);
+ }
+
+ /**
+ * Subcase-A regression for a non-self-transitionable convergence target
(PHOENIX-7990). Unlike
+ * {@link #testSetHAGroupStatusIfNeededConvergentRaceReconciles}, which
converges on
+ * ACTIVE_NOT_IN_SYNC (the one self-transitionable state, so it reconciles
even without the fix),
+ * this drives STANDBY -> STANDBY_TO_ACTIVE: an allowed, ungated transition
whose target is NOT
+ * self-transitionable. A peer wins the CAS to STANDBY_TO_ACTIVE while this
client's cache is
+ * still stale at STANDBY; attempt 1 loses the stale CAS, re-reads fresh
(now STANDBY_TO_ACTIVE),
+ * and the no-op short-circuit — which the fix moved ahead of
validateTransitionAndGetWaitTime —
+ * returns a no-op success. This is the case the reorder guards: revert it
and attempt 2's
+ * validate(STANDBY_TO_ACTIVE -> STANDBY_TO_ACTIVE) throws
InvalidClusterRoleTransitionException
+ * on the X -> X self-transition.
+ */
+ @Test
+ public void
testSetHAGroupStatusIfNeededConvergentRaceNonSelfTransitionableTarget()
Review Comment:
Isn't there a possibility of test being flaky?
What if attempt 1 reads the cache and not ZK. The no-op short-circuit is
attempt > 1 only. So if the client's PathChildrenCache watch delivers the
peer's STANDBY_TO_ACTIVE write before the test thread calls
setHAGroupStatusIfNeeded, attempt 1 hits validate(STANDBY_TO_ACTIVE →
STANDBY_TO_ACTIVE) so it's not self-transitionable and throw
InvalidClusterRoleTransitionException.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
