xiangfu0 commented on code in PR #20107:
URL: https://github.com/apache/pinot/pull/20107#discussion_r4429520895
##########
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:
**Minor: a mixed snapshot has the same ambiguity.** In Helix 2.0.1,
`CallbackHandler.handleDataDeleted` drops a child's data watch without calling
the listener. Only a child-change `invoke` subscribes it again. A config write
between those two points is visible only to this scan.
Example: a server is dropped and re-added under the same name, and a minion
join lands in the same coalesced callback. `changedInstances` is then
`{Minion_x}`, so the scan is skipped. If its `IS_SHUTDOWN_IN_PROGRESS=true`
write is missed, the later `false` doesn't refresh the channel. A local probe
confirms this: base refreshes the channel and this head doesn't.
The window is narrow, so this isn't a blocker. Could you document the
accepted gap here, or pin the behavior with a test?
##########
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:
**Minor: test gap.** No test covers the retry after a failed read. A local
probe passes at this head: the INIT read throws, a minion child change reads
the config again, and the later `false` callback refreshes the channel. Worth
adding as a test, since this line is what stops a later skip from hiding a
failed read.
--
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]
