yashmayya opened a new pull request, #20005:
URL: https://github.com/apache/pinot/pull/20005

   > **Stacked on #19875.** This PR uses the `_dispatchAddressByInstanceId` map 
that #19875 adds, so the branch also has the #19875 commit. Until #19875 
merges, review only the last commit: "Recover unhealthy MSE workers that are 
not servers in the routing manager".
   
   ## Problem
   
   The broker failure detector (FD) marks an instance unhealthy when an MSE 
dispatch to it fails. Some of these instances are not enabled servers in the 
local routing manager:
   
   - servers of a remote cluster, with multi-cluster routing (#17444)
   - workers that a custom `WorkerManager` picks, through 
`BaseBrokerStarter.createWorkerManager` (#17787)
   
   The FD cannot recover such an instance. Each retrier gets the server from 
the local `RoutingManager.getEnabledServerInstanceMap()`. If the id is not in 
this map, the retrier returns `UNHEALTHY`, and the FD stops at the first 
`UNHEALTHY` retrier. As a result, the instance stays unhealthy until the last 
retry: about 2 h 50 min with the default config. 
`BrokerGauge.UNHEALTHY_SERVERS` counts it for all of that time.
   
   The `FailureDetector` SPI already says that a retrier returns `UNKNOWN` for 
a server that it does not know.
   
   ## Fix
   
   - The single-stage retriers (Netty and gRPC) return `UNKNOWN` for an id that 
is not an enabled server in the routing manager.
   - The MSE retrier checks the dispatch channel to an instance at the best 
address that the broker has:
     1. An enabled server of the local cluster: its address in the routing 
manager, as before.
     2. With multi-cluster routing, an enabled server of a remote cluster: its 
address in the multi-cluster routing manager.
     3. Any other id: the address that a query was last dispatched to. 
`QueryDispatcher` records it in `_dispatchAddressByInstanceId`, which #19875 
adds. Only a dispatch records it, not a cancel.
   - If the broker never dispatched to the id, the MSE retrier returns 
`UNKNOWN`.
   
   #15116 removed an earlier dispatch-time address map, because that address 
goes stale when a server restarts on a new port. So this PR uses the 
dispatch-time address only for ids that no routing manager knows. For these 
ids, the address stays stale until the next dispatch, or until a call to 
`QueryDispatcher.onServerEnabled`. Before this PR, these ids could not recover 
at all.
   
   ## Compatibility
   
   - No wire, config or metric change.
   - New method: `QueryDispatcher.checkConnectivityToInstance(String 
instanceId)`.
   - A disabled server that is unhealthy can now leave the FD before it is 
enabled again. This occurs when no retrier knows it, or when the dispatch 
channel to its last address is READY. Routing does not change, because 
`includeServerToRouting` skips servers that are not enabled.
   - The single-stage retriers still do not check remote-cluster servers. A 
remote-cluster server that a single-stage query marked unhealthy now leaves the 
FD at the first retry, unless the MSE retrier finds its dispatch channel not 
READY. This does not change routing, because the FD notifiers update only the 
local routing manager.
   
   ## Tests
   
   - `QueryDispatcherTest`:
     - An id outside the routing manager is `UNKNOWN` before a dispatch, 
`UNHEALTHY` while its channel is not READY, and `HEALTHY` after its server 
starts.
     - A cancel does not change the recorded address. A dispatch to a new port 
does.
     - A real dispatch records the address of each server.
   - `MultiStageBrokerRequestHandlerTest`:
     - The retrier checks local and remote-cluster servers at their current 
address, and other ids at the recorded address.
     - A real `ConnectionFailureDetector` with the retriers of all three 
request handlers keeps such a worker unhealthy while its channel is not READY. 
Then it marks the worker healthy. The test fails without the single-stage part 
or the MSE part of the fix.
   - The `pinot-query-runtime` and `pinot-broker` test suites 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]

Reply via email to