lokiore commented on code in PR #2490:
URL: https://github.com/apache/phoenix/pull/2490#discussion_r3337261599
##########
phoenix-core-client/src/main/java/org/apache/phoenix/util/GetClusterRoleRecordUtil.java:
##########
@@ -181,34 +208,47 @@ private static void schedulePoller(String url, String
haGroupName, HighAvailabil
}
schedulerMap.put(haGroupName, Executors.newScheduledThreadPool(1));
- LOGGER.info("Starting poller for HA group {} to check every {}
milliseconds.", haGroupName,
- pollerInterval);
+ LOGGER.info(
+ "Starting poller for HA group {} to check every {} milliseconds,
alternating between {}"
+ + " and {}.",
+ haGroupName, pollerInterval, url1, url2);
+ AtomicLong tickCount = new AtomicLong(0);
Runnable pollingTask = () -> {
+ // Increment unconditionally so a failed tick still alternates next
iteration.
+ long tick = tickCount.getAndIncrement();
+ String tickUrl = selectUrlForTick(url1, url2, tick);
try {
- ClusterRoleRecord polledCrr = getClusterRoleRecord(url, haGroupName,
true, properties);
- LOGGER.info("Polled CRR: {}", polledCrr);
+ ClusterRoleRecord polledCrr =
+ getClusterRoleRecord(tickUrl, haGroupName, true, properties);
+ LOGGER.info("Polled CRR for HA group {} via {}: {}", haGroupName,
tickUrl, polledCrr);
if (polledCrr.getRole1().isActive() ||
polledCrr.getRole2().isActive()) {
- LOGGER.info("Active ClusterRoleRecord found. Cancelling poller.");
+ LOGGER.info("Active ClusterRoleRecord found for HA group {}.
Cancelling poller.",
+ haGroupName);
synchronized (pollerLock) {
- if (pollerFuture != null) {
- pollerFuture.cancel(false);
+ ScheduledFuture<?> future = futureMap.remove(haGroupName);
+ if (future != null) {
+ future.cancel(false);
}
// Refresh ClusterRoleRecord for the HAGroup with appropriate
transition
haGroup.refreshClusterRoleRecord(true);
Review Comment:
Good catch, fixed in 092a5388c0. Wrapped
`haGroup.refreshClusterRoleRecord(true)` in `try/finally` so the `schedulerMap`
removal + `scheduler.shutdown()` always run even if refresh throws — preserves
the original "found ACTIVE → consume role record → release resources" semantic
intent and closes the leak path you identified.
FYI @tkhurana
_(Posted by Loki's Claude companion)_
--
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]
