xiangfu0 commented on code in PR #20107:
URL: https://github.com/apache/pinot/pull/20107#discussion_r4429923825
##########
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:
Fixed in b4a91b4f5b. Skipping now requires a coherent parent snapshot, the
same parent czxid, and a cversion advance exactly equal to the net non-server
name changes. Helix 2.0.1 has no public atomic child-name/Stat API, so the
existing callback manager supplies a Stat → names → Stat bracket; changing or
unavailable metadata forces a full scan. Added regressions for coalesced server
recreation, parent recreation, counter wrap, and an unstable bracket. The
handler constructor is unchanged.
##########
pinot-server/src/main/java/org/apache/pinot/server/starter/helix/ServerMailboxChannelRefreshHandler.java:
##########
@@ -145,6 +159,8 @@ private void handleFullScan() {
}
_shuttingDownServers.clear();
_shuttingDownServers.addAll(currentShuttingDown);
+ // A failed read must not suppress a retry on the next child change.
+ _instances = allConfigsRead ? instances : Set.of();
Review Comment:
Added testFailedInitReadRetriesOnMinionMembershipChange in b4a91b4f5b: the
INIT config read fails, a minion membership change retries and observes
shutdown=true, and the later readiness=false callback refreshes the channel.
This passes in the 26-test handler suite.
##########
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:
Fixed in b4a91b4f5b. Null or failed config reads now invalidate the
membership snapshot on the individual callback path too. The next child
callback therefore retries instead of skipping. Full scans also retain
previously observed shutdown state when a retry read fails. Regression coverage
verifies the requested sequence and a repeated-failure variant; both now
refresh the channel.
--
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]
