xiangfu0 commented on code in PR #20021:
URL: https://github.com/apache/pinot/pull/20021#discussion_r4366297505
##########
pinot-broker/src/main/java/org/apache/pinot/broker/routing/manager/BaseBrokerRoutingManager.java:
##########
@@ -520,6 +535,7 @@ private void processInstanceConfigChangeInternal() {
// always contains the selected servers
for (String newDisabledInstance : newDisabledServers) {
_enabledServerInstanceMap.remove(newDisabledInstance);
+ _previouslyEnabledServers.add(newDisabledInstance);
Review Comment:
Done. The map (now `instanceId -> last known ServerInstance`) only holds
servers that are still in the cluster but disabled or shutting down. When a
disabled server's instance config disappears, the entry is dropped and the new
`addServerRemovedCallback` listeners are told; a server that goes from enabled
to removed in one step is reported the same way without ever entering the map.
The single-stage handler uses that callback to evict its cached channels to the
removed server (`BrokerRequestHandler#onServerRemoved`), so a server added back
under the same id starts on a fresh connection even though it counts as new
rather than re-enabled. The prune runs before the early return, since a removal
changes no routing. Covered by
`BrokerRoutingManagerTest#testRemovedServerIsForgottenAndRemovedCallbackInvoked`;
the existing re-enable tests now model a disable as a Helix-disabled instance
config rather than a missing one.
##########
pinot-broker/src/main/java/org/apache/pinot/broker/routing/manager/BaseBrokerRoutingManager.java:
##########
@@ -451,14 +475,16 @@ private void processInstanceConfigChangeInternal() {
newEnabledServers.add(instanceId);
// NOTE: Remove new enabled server from excluded servers because
the server is likely being restarted
- if (_excludedServers.remove(instanceId)) {
+ boolean wasExcluded = _excludedServers.remove(instanceId);
+ if (wasExcluded) {
LOGGER.info("Got excluded server: {} re-enabled, including it
into the routing", instanceId);
- // We clear any GRPC channel reconnection backoff in this
callback when a server is re-enabled. Otherwise,
- // MSE queries to this server may fail fast until the next
backoff retry succeeds.
- if (_serverReenableCallback != null) {
- LOGGER.info("Calling server re-enable callback for server:
{}", instanceId);
- _serverReenableCallback.accept(serverInstance);
- }
+ }
+ // A server that comes back after having left the enabled set has
most likely been restarted, whether or
+ // not the failure detector had excluded it in between. Let the
listeners drop state tied to the previous
+ // process: cached Netty channels that may be half-open, and the
gRPC connection backoff that would
+ // otherwise make MSE queries fail fast until the next backoff
retry succeeds.
+ if (_previouslyEnabledServers.remove(instanceId) | wasExcluded) {
Review Comment:
Agreed, that was too clever. The excluded-set removal already happened in
its own statement a few lines up (`wasExcluded`), so the second removal is now
its own statement too (`wasDisabled`) and the condition is a plain `||`.
--
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]
