tkhurana commented on PR #2589: URL: https://github.com/apache/phoenix/pull/2589#issuecomment-5332586790
## Review — dual-endpoint CRR reconcile + refresh guard **Verdict: approve with minor follow-ups.** Core logic is correct on the three highest-risk areas — the reconcile decision tree, the AB-BA deadlock fix, and poller-scheduling behavior. No correctness or concurrency defects found in the new code. Items below are a doc fix, diagnostics gaps, and test wiring. ### Verified solid - **Reconcile is sound.** The equal-version-divergence defer fires only when `current != null && hasSameInfo && current.version == endpoints' version`, so a genuine version advance is never suppressed; when it defers it returns the *same object* as `this.roleRecord`, so the downstream `equals()` short-circuits to a no-op (no flap). `getClusterRoleRecordFromEndpoint` is only called under the write lock, so the `this.roleRecord` read is safe. - **AB-BA deadlock genuinely resolved.** Connect path takes writeLock→pollerLock; the old tick took pollerLock→writeLock via in-lock refresh. Moving `refreshClusterRoleRecord` outside `synchronized(pollerLock)` removes the inversion. "Exactly once" holds: single-thread `scheduleWithFixedDelay` never overlaps ticks, and `futureMap.remove()` under `pollerLock` elects exactly one winner. - No poller-scheduling regression — gating on the *reconciled* record is more correct than the old per-raw-fetch scheduling. Import restrictions clean; all log placeholders match args. ### Important (fix before merge) 1. **Comment contradicts the code it documents** — `HighAvailabilityGroup.java:1314-1315` (mirrored at `HighAvailabilityGroupTest.java:451-454`). The newer-UNKNOWN-with-no-active carve-out says "the non-active poller picks up the true state on its next tick," but that branch returns `usableRecord`; when the usable record has an active role (the tested case — `v9` is ACTIVE/STANDBY), `maybeSchedulePoller` is a no-op, so no poller runs — recovery comes from the next time-based refresh. Suggest: "recovery comes from the next scheduled refresh (and the poller only if the usable record is itself non-active)." ### Concerns (diagnostics — new code) 2. **Outer catch never logs the cluster-1 exception** — `HighAvailabilityGroup.java:1034-1052`. On a non-NOT-FOUND cluster-1 failure, the code retries cluster 2 with no log of why cluster 1 failed; if cluster 2 succeeds, the cluster-1 failure (including an unchecked bug like an NPE) vanishes. Contrast the inner catch at `:1028`, which logs `e`. Recommend a WARN with `e` before the fallback, plus the same `isCausedByInterrupt` treatment. 3. **Reconcile runs inside the cluster-2 try block** — `HighAvailabilityGroup.java:1008-1033`. `reconcileClusterRoleRecords` (`:1012`) is pure local computation but sits inside the `catch (Exception)` that reports "cluster 2 endpoint threw an exception." A reconcile bug would be misreported as peer-unreachable and silently degraded to cluster-1's record. Narrow the try to just the cluster-2 fetch; run reconcile after. 4. **Dropped `ignoredEx`** — `HighAvailabilityGroup.java:1044`. Rethrowing the original NOT-FOUND is intentional/correct, but the cluster-2 failure reason is discarded. Prefer `((SQLException) e).addSuppressed(ignoredEx)`. ### Pre-existing (adjacent to the touched poller block — not introduced here; suggest a follow-up ticket) - Poller tick catches only `SQLException` (`GetClusterRoleRecordUtil.java:272`); an unchecked exception escaping `scheduleWithFixedDelay` silently kills the poller forever. The metric-sampling path already uses `catch (Throwable)` — the primary path should too. - If the winner tick's `refreshClusterRoleRecord` throws, the scheduler is already torn down and nothing reschedules (self-destruct predates the refactor). Since this PR restructured that block, a good moment to re-arm or log ERROR distinctly. - Poller catch logs `e.getMessage()` with no stack trace (`:274-276`). ### Test coverage Helpers are well-covered — both arg orders, version ties, the strict-`>` active-UNKNOWN boundary (explicit `>=`-mutant killer at `HighAvailabilityGroupTest.java:462-467`), and the anti-flap defer. Gaps by priority: 1. **The refresh no-rollback branch is untested** (`:1141-1149`) — the PR's whole point. A passing `shouldApplyRefreshedRecord` unit test doesn't prove the branch is wired (keeps `roleRecord`, stays `READY`, no failover count, returns `true`). An inverted guard would silently reintroduce the rollback with all helper tests still green. 2. **`getClusterRoleRecordFromEndpoint` wiring untested** — reconcile is order-independent, so a cluster-1/cluster-2 arg swap or wrong `current` would pass every helper test but change first-load behavior. 3. **`isCausedByInterrupt` untested** — cheap/pure; pin the depth-16 cycle bound and both exception types. 4. Equal-version fall-through covers only `current == null`, not `current` at a *lower* version than the diverging endpoints (`:1344-1352`). 5. No IT drives the divergent/lagging-endpoint scenario — existing ITs move both endpoints consistently. Nothing here is a correctness blocker. _Reviewed with assistance from Claude Code (Opus 4.8)._ -- 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]
