Copilot commented on code in PR #2600:
URL: https://github.com/apache/phoenix/pull/2600#discussion_r3789327910
##########
phoenix-core-server/src/main/java/org/apache/phoenix/replication/ReplicationLogDiscoveryForwarder.java:
##########
@@ -156,32 +161,42 @@ && isLogCopyThroughputAboveThreshold(srcStat.getLen(),
copyTime)
@Override
protected void processNoMoreRoundsLeft() throws IOException {
- // check if we are caught up so that we can transition to SYNC state
- // we are caught up when there are no files currently in the out progress
directory
- // and no new files exist for ongoing round
+ // A non-empty in-progress directory means this RS has claimed-but-stuck
files, so its forward
+ // path to the peer is unhealthy: neither promote its own mode nor claim
the group is in sync.
+ if (!replicationLogTracker.getInProgressFiles().isEmpty()) {
+ LOG.info("In-progress directory not empty for {}, skipping mode
promotion and sync claim",
+ logGroup);
+ return;
+ }
+
+ // No stuck files, so promote this RS's own mode on that signal alone.
Gating on the next
+ // round's
+ // shard would pin an idle RS forever: it is a shared directory holding
every co-active RS's
+ // live
+ // rotation writer. The flip is self-validating —
SyncAndForwardModeImpl.onEnter must reach the
+ // peer, so a bad promotion bounces back to STORE_AND_FORWARD.
+ logGroup.checkAndSetModeAndNotify(STORE_AND_FORWARD, SYNC_AND_FORWARD);
+
+ // The shared in-sync claim additionally requires no new files for the
ongoing round.
if (
- replicationLogTracker.getInProgressFiles().isEmpty()
- && replicationLogTracker.getNewFilesForRound(replicationLogTracker
-
.getReplicationShardDirectoryManager().getNextRound(getLastRoundProcessed())).isEmpty()
+ !replicationLogTracker.getNewFilesForRound(replicationLogTracker
+
.getReplicationShardDirectoryManager().getNextRound(getLastRoundProcessed())).isEmpty()
) {
- LOG.info("Processed all the replication log files for {}", logGroup);
- // if this RS is still in STORE_AND_FORWARD mode like when it didn't
process any file
- // move this RS to SYNC_AND_FORWARD
- logGroup.checkAndSetModeAndNotify(STORE_AND_FORWARD, SYNC_AND_FORWARD);
-
- if (syncUpdateTS <= EnvironmentEdgeManager.currentTimeMillis()) {
- try {
- long waitTime = logGroup.setHAGroupStatusToSync();
- if (waitTime != 0) {
- syncUpdateTS = EnvironmentEdgeManager.currentTimeMillis() +
waitTime;
- LOG.info("HAGroup {} will try to update HA state to sync at {}",
logGroup,
- syncUpdateTS);
- } else {
- LOG.info("HAGroup {} updated HA state to SYNC", logGroup);
- }
- } catch (Exception e) {
- LOG.info("Could not update status to sync for {}", logGroup, e);
+ LOG.info("New files present for the next round for {}, skipping sync
claim", logGroup);
+ return;
+ }
+ LOG.info("Processed all the replication log files for {}", logGroup);
+ if (syncUpdateTS <= EnvironmentEdgeManager.currentTimeMillis()) {
+ try {
+ long waitTime = logGroup.setHAGroupStatusToSync();
Review Comment:
The optimistic promotion is not validated before this shared SYNC claim.
`checkAndSetModeAndNotify()` waits on a sync future, but
`ReplicationLogGroup.processPendingSyncs()` completes that future before it
calls `initializeMode(newMode)` (lines 1390-1410), so this thread can call
`setHAGroupStatusToSync()` while `SyncAndForwardModeImpl.onEnter()` is still
failing and concurrently restoring STORE_AND_FORWARD. If the SYNC update wins
that race, the group advertises ACTIVE_IN_SYNC with an unreachable peer. Please
gate this claim on confirmed completion of the mode transition, and make the
peer-unreachable test verify that no SYNC claim survives.
--
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]
