Jackie-Jiang commented on code in PR #20107:
URL: https://github.com/apache/pinot/pull/20107#discussion_r4429854545
##########
pinot-server/src/main/java/org/apache/pinot/server/starter/helix/ServerMailboxChannelRefreshHandler.java:
##########
@@ -103,23 +107,33 @@ public synchronized void
onInstanceConfigChange(List<InstanceConfig> instanceCon
}
}
- /// Full cluster scan: used on INIT (first callback) and when instances are
added/removed.
+ /// Scans on INIT and child changes, skipping membership changes confined to
non-server instances.
///
/// It records the new address of every server before it retires any old
one. Otherwise, when one server left an
/// address and a server read later in the scan took it over, the scan would
shut down the channel the second server
/// uses.
- private void handleFullScan() {
+ private void handleFullScan(boolean initialize) {
+ Set<String> instances = new
HashSet<>(_helixAdmin.getInstancesInCluster(_clusterName));
+ Set<String> changedInstances = Sets.symmetricDifference(_instances,
instances);
+ // An unchanged snapshot is ambiguous (e.g. a server was recreated with
the same name), so still scan it.
+ if (!initialize && !changedInstances.isEmpty()
+ && changedInstances.stream().noneMatch(InstanceTypeUtils::isServer)) {
Review Comment:
[P1] This needs a correctness fix before merge, rather than documentation of
the gap. I independently traced the same interleaving through Helix 2.0.1 and
ZooKeeper: the deletion consumes the parent's one-shot watch, and the server
recreation plus Minion addition can both occur before that watch is rearmed.
There need not be another child callback to repair the missed startup state.
The later readiness=false callback then leaves the old mailbox channel
unrefreshed, potentially continuing to use a stale DNS destination.
One way to preserve the optimization is to capture the participant-parent
cversion atomically with the child-name snapshot, and skip only when its
advance equals the number of net changed names and all those names are
nonservers. Otherwise a hidden delete/recreate requires a scan. Explicit
server-deletion tracking is another option. Please add regression coverage for
server recreation combined with a Minion addition.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
