ritegarg commented on code in PR #2479:
URL: https://github.com/apache/phoenix/pull/2479#discussion_r3291061141
##########
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
+ * remove so that, if a concurrent {@link #getInstanceForZkUrl} has already
swapped in a fresh
+ * replacement, the replacement is preserved.
+ */
+ private void deregisterFromInstances() {
+ final String key = (this.zkUrl != null) ? this.zkUrl :
getLocalZkUrl(this.conf);
+ if (key == null) {
+ return;
+ }
+ final ConcurrentHashMap<String, HAGroupStoreClient> bucket =
instances.get(key);
+ if (bucket == null) {
+ return;
+ }
+ bucket.remove(this.haGroupName, this);
Review Comment:
Race is real — the deregister's bucket-removal can interleave between
getInstanceForZkUrl's putIfAbsent and get. Fixing by making registration atomic
at the ConcurrentHashMap level instead: replace the putIfAbsent +
get().put(...) pair with a single instances.compute(localZkUrl, (k, v) -> ...)
callback that creates the bucket if absent and inserts the new client in one
CHM-atomic step. deregisterFromInstances's existing computeIfPresent is already
CHM-atomic on the same key, so the two operations serialize via the outer CHM's
per-bin lock; no new monitor needed. I considered the alternative of
synchronizing deregisterFromInstances on HAGroupStoreClient.class to match the
registration monitor, but it would make close() block on any in-flight
constructor — including the slow paths (cache latch.await(...), JDBC bootstrap,
peer cache init, NodeCache.start(true)) — which would worsen the head-of-line
issue you raised on the NodeCache.start thread. Added a short comment at both ca
ll sites describing the CHM pairing so the invariant is locally visible.
--
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]
