xiangfu0 commented on code in PR #19875:
URL: https://github.com/apache/pinot/pull/19875#discussion_r4289839924
##########
pinot-server/src/main/java/org/apache/pinot/server/starter/helix/ServerMailboxChannelRefreshHandler.java:
##########
@@ -95,18 +106,28 @@ public synchronized void
onInstanceConfigChange(List<InstanceConfig> instanceCon
/// Full cluster scan: used on INIT (first callback) and when instances are
added/removed.
private void handleFullScan() {
Set<String> currentShuttingDown = new HashSet<>();
+ Set<String> currentServers = new HashSet<>();
for (String instance : _helixAdmin.getInstancesInCluster(_clusterName)) {
if (!InstanceTypeUtils.isServer(instance) ||
instance.equals(_selfInstanceId)) {
continue;
}
+ // Before reading the config: a server whose config cannot be read now
has not left the cluster
+ currentServers.add(instance);
InstanceConfig config = getInstanceConfig(instance);
if (config == null) {
continue;
}
+ updateMailboxAddress(instance, config);
Review Comment:
During a full scan `updateMailboxAddress` is applied one instance at a time,
so the `containsValue` guard in `retireChannel` reads a half-updated
`_mailboxAddresses`. If server A (scanned first) vacates an address that server
B (scanned later in the same scan) takes over, A's retire runs while B still
maps to its old address, so the guard misses the takeover and shuts down a
channel B will use. It self-heals via lazy recreate, and the single-instance
path is fine, but the full-scan ordering isn't covered by a test. Worth a
comment or a two-pass scan.
--
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]
