xiangfu0 commented on code in PR #20107:
URL: https://github.com/apache/pinot/pull/20107#discussion_r4430142795
##########
pinot-server/src/main/java/org/apache/pinot/server/starter/helix/ServerMailboxChannelRefreshHandler.java:
##########
@@ -68,6 +74,12 @@ public class ServerMailboxChannelRefreshHandler implements
InstanceConfigChangeL
private final String _selfInstanceId;
private final MailboxService _mailboxService;
+ // A coherent participant snapshot, invalidated whenever a membership or
instance config read fails.
+ private Set<String> _instances = Set.of();
+ private long _instancesParentCzxid;
+ private int _instancesParentCversion;
+ private boolean _instancesSnapshotValid;
Review Comment:
Addressed in db39ab9106. The cache is now one nullable immutable
MembershipSnapshot with a defensive copy of the names and scalar parent
czxid/cversion. Each membership callback captures and clears the old snapshot,
then publishes a replacement only after a proven skip or successful coherent
scan. Failed config reads invalidate it with one assignment. All 26 handler
tests and the module style/license checks pass.
##########
pinot-server/src/main/java/org/apache/pinot/server/starter/helix/ServerMailboxChannelRefreshHandler.java:
##########
@@ -103,23 +115,62 @@ 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(NotificationContext context) {
Review Comment:
Addressed in db39ab9106. Renamed the entry point to handleMembershipChange
and extracted scanServerConfigs(instances), which returns whether every config
was read successfully. Metadata reads, the skip decision, and snapshot
publication remain together. Unstable metadata still runs the scan, and the
existing recovery and channel-retirement ordering are unchanged. All 26 handler
tests pass.
##########
pinot-server/src/test/java/org/apache/pinot/server/starter/helix/ServerMailboxChannelRefreshHandlerTest.java:
##########
@@ -206,12 +223,163 @@ public void testInitCallbackDoesFullScanAndTracksState()
{
verify(_mailboxService).refreshChannel(OTHER_SERVER_HOST,
OTHER_SERVER_MAILBOX_PORT);
}
+ @Test
+ public void testMinionMembershipChangesDoNotReadServerConfigs() {
+ String minionId = "Minion_minionhost_8000";
+ stubParticipantSnapshot(List.of(SELF_INSTANCE_ID, OTHER_SERVER_ID), 10, 1);
+ when(_helixAdmin.getInstanceConfig(CLUSTER_NAME, OTHER_SERVER_ID))
+ .thenReturn(createServerConfig(OTHER_SERVER_ID, OTHER_SERVER_HOST,
OTHER_SERVER_MAILBOX_PORT, true));
+ _handler.onInstanceConfigChange(List.of(), createInitContext());
+
+ stubParticipantSnapshot(List.of(minionId, OTHER_SERVER_ID,
SELF_INSTANCE_ID), 11, 1);
+ _handler.onInstanceConfigChange(List.of(), createChildChangeContext());
+ stubParticipantSnapshot(List.of(SELF_INSTANCE_ID, OTHER_SERVER_ID), 12, 1);
+ _handler.onInstanceConfigChange(List.of(), createChildChangeContext());
+ verify(_helixAdmin, times(1)).getInstanceConfig(CLUSTER_NAME,
OTHER_SERVER_ID);
+ verify(_mailboxService, never()).refreshChannel(any(), anyInt());
+
+ when(_helixAdmin.getInstanceConfig(CLUSTER_NAME, OTHER_SERVER_ID))
+ .thenReturn(createServerConfig(OTHER_SERVER_ID, OTHER_SERVER_HOST,
OTHER_SERVER_MAILBOX_PORT, false));
+ _handler.onInstanceConfigChange(List.of(),
createCallbackContextForInstance(OTHER_SERVER_ID));
+ verify(_mailboxService).refreshChannel(OTHER_SERVER_HOST,
OTHER_SERVER_MAILBOX_PORT);
+ }
+
+ @Test
+ public void testServerMembershipChangesStillScanConfigs() {
+ stubParticipantSnapshot(List.of(SELF_INSTANCE_ID));
+ when(_helixAdmin.getInstanceConfig(CLUSTER_NAME, OTHER_SERVER_ID))
+ .thenReturn(createServerConfig(OTHER_SERVER_ID, OTHER_SERVER_HOST,
OTHER_SERVER_MAILBOX_PORT, true));
+ _handler.onInstanceConfigChange(List.of(), createInitContext());
+ stubParticipantSnapshot(List.of(SELF_INSTANCE_ID, OTHER_SERVER_ID));
Review Comment:
Addressed in db39ab9106. Both testServerMembershipChangesStillScanConfigs
and testRetiresChannelOfServerThatLeftTheCluster now use cversion 10 then 11,
so the server-type guard determines that these membership changes must scan. No
new tests added; all 26 existing handler test cases pass.
--
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]
