Jackie-Jiang commented on code in PR #20107:
URL: https://github.com/apache/pinot/pull/20107#discussion_r4429854882
##########
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)) {
+ _instances = instances;
+ return;
Review Comment:
[P1] Invalidate the cache after individual config-read failures
If INIT records a server as shutting down, then its readiness callback fails
to read the config, handleSingleInstanceChange returns without invalidating
_instances. Subsequent Minion membership changes now skip the scan that
previously retried the read and refreshed the channel. Helix sees normal
callback completion because getInstanceConfig catches the exception, so it does
not retry. Continuing non-server membership churn can leave the channel stale
until another relevant server event or INIT.
Invalidate the cached snapshot, or set a dirty flag, after individual read
failures too. Please add regression coverage for INIT observing shutdown=true
-> readiness data callback fails to read -> Minion joins -> config read
succeeds with shutdown=false -> channel refreshes.
--
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]
