Copilot commented on code in PR #2479:
URL: https://github.com/apache/phoenix/pull/2479#discussion_r3270086534
##########
phoenix-core-client/src/main/java/org/apache/phoenix/jdbc/HAGroupStoreClient.java:
##########
@@ -886,9 +913,16 @@ private PathChildrenCacheListener
createCacheListener(CountDownLatch latch,
if (cacheType == ClusterType.LOCAL) {
maybeInitializePeerPathChildrenCache();
}
+ // Offload the legacy CRR sync (it does ZK + JDBC I/O) so we don't
block
+ // Curator's per-namespace event dispatcher.
+ ScheduledExecutorService syncExec = legacyCrrSyncExecutor;
+ if (syncExec != null) {
+ syncExec.execute(this::syncLegacyCRRIfRoleChanged);
Review Comment:
The listener offloads legacy CRR sync by calling `syncExec.execute(...)`
without guarding against `RejectedExecutionException`. If `close()` is racing
with Curator event delivery, the executor may already be shutting down and this
unchecked exception would escape Curator’s event thread. Catch
`RejectedExecutionException` (or check `!isShutdown()` and still catch) and
treat it as a no-op during shutdown.
##########
phoenix-core-client/src/main/java/org/apache/phoenix/jdbc/ClusterRoleRecord.java:
##########
@@ -221,6 +241,20 @@ public boolean hasSameInfo(ClusterRoleRecord other) {
return haGroupName.equals(other.haGroupName) &&
policy.equals(other.policy);
}
+ /**
+ * Equality on the six identity/role fields ({@code haGroupName, policy,
url1, url2, role1,
+ * role2}); ignores {@code version} (always bumps) and {@code registryType}
(avoids RPC->ZK
+ * thrash). Returns {@code false} if {@code other} is {@code null}.
+ */
+ public boolean isLogicallyEqualIgnoringVersionAndRegistry(ClusterRoleRecord
other) {
+ if (other == null) {
+ return false;
+ }
+ return Objects.equals(haGroupName, other.haGroupName) &&
Objects.equals(policy, other.policy)
+ && Objects.equals(url1, other.url1) && Objects.equals(url2, other.url2)
+ && role1 == other.role1 && role2 == other.role2;
+ }
Review Comment:
The Javadoc says this helper “ignores registryType (avoids RPC->ZK thrash)”,
but the implementation compares `url1`/`url2`, which are already normalized
based on `registryType`, so ZK vs RPC records will still be considered
different. Please adjust the comment to match actual behavior (e.g., it ignores
the `registryType` field only), or change the logic if you truly want equality
across registry-type URL normalization.
##########
phoenix-core-client/src/main/java/org/apache/phoenix/jdbc/HAGroupStoreClient.java:
##########
@@ -983,32 +1017,65 @@ private void closePeerConnection() {
/**
* Shuts down the periodic sync executor gracefully.
*/
+ /**
+ * Remove this instance from the static {@link #instances} map. Idempotent.
Uses value-based
Review Comment:
There are two consecutive Javadoc blocks here; the first one (“Shuts down
the periodic sync executor gracefully.”) is orphaned and no longer documents
any method. Remove it or merge it into the correct method’s Javadoc to avoid
confusing/incorrect documentation.
--
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]
