yashmayya commented on code in PR #20005:
URL: https://github.com/apache/pinot/pull/20005#discussion_r4379632037
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/service/dispatch/QueryDispatcher.java:
##########
@@ -899,6 +920,20 @@ DispatchClient
getOrCreateDispatchClient(QueryServerInstance queryServerInstance
_dispatchMaxInboundMessageSizeBytes));
}
+ /// Records the address of an instance that a query is dispatched to, as its
last known address, see
+ /// [#checkConnectivityToInstance(String)]. Only dispatches record it: a
cancel can still go to an address that the
+ /// instance has left.
+ @VisibleForTesting
+ void recordDispatchAddress(QueryServerInstance queryServerInstance) {
+ String instanceId = queryServerInstance.getInstanceId();
+ String hostnamePortKey =
+ toHostnamePortKey(queryServerInstance.getHostname(),
queryServerInstance.getQueryServicePort());
+ // Read first, so that a dispatch to an instance that did not move does
not write the map
+ if (!hostnamePortKey.equals(_dispatchAddressByInstanceId.get(instanceId)))
{
+ _dispatchAddressByInstanceId.put(instanceId, hostnamePortKey);
Review Comment:
Thanks. I added a note on the field in f0a1e5fd20. Entries are never
removed, but there is one per instance id, so the number of instances bounds
the map while ids are stable, as Helix ids are. `onServerEnabled` scans the
values only when a server is enabled, which is rare. Removing an entry when an
instance leaves needs a leave event that `QueryDispatcher` does not get today,
so I left that for a follow-up.
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/SingleConnectionBrokerRequestHandler.java:
##########
@@ -1048,9 +1048,11 @@ public FailureDetector.ServerState
retryUnhealthyServer(String instanceId) {
LOGGER.info("Retrying unhealthy server: {}", instanceId);
ServerInstance serverInstance =
_routingManager.getEnabledServerInstanceMap().get(instanceId);
+ // Not an enabled server of the routing manager, for example a disabled
server or a server of a remote cluster.
+ // Leave it to the other retriers.
if (serverInstance == null) {
LOGGER.info("Failed to find enabled server: {} in routing manager,
skipping the retry", instanceId);
- return FailureDetector.ServerState.UNHEALTHY;
+ return FailureDetector.ServerState.UNKNOWN;
Review Comment:
Yes, that is intended. When MSE is disabled, this is the only retrier, so
such an id now leaves the FD at the first retry instead of after the last one.
Routing does not change, because `includeServerToRouting` skips servers that
are not enabled. I added this to the Compatibility section of the description.
--
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]
