Jackie-Jiang commented on code in PR #19178:
URL: https://github.com/apache/pinot/pull/19178#discussion_r3787566135
##########
pinot-broker/src/main/java/org/apache/pinot/broker/routing/manager/BaseBrokerRoutingManager.java:
##########
@@ -1258,6 +1258,19 @@ public Set<String> getServingInstances(String
tableNameWithType) {
return routingEntry._instanceSelector.getServingInstances();
}
+ /// Returns whether the server is currently available to the broker routing
entries.
+ ///
+ /// The read lock ensures this method cannot observe
`_routableServerInstanceMap` while an instance-config callback
+ /// is still applying the corresponding change to individual routing entries.
+ public boolean isServerRoutable(String instanceId) {
+ _globalLock.readLock().lock();
Review Comment:
I don't think the lock is required
##########
pinot-broker/src/main/java/org/apache/pinot/broker/api/resources/PinotBrokerHealthCheck.java:
##########
@@ -75,9 +81,18 @@ public class PinotBrokerHealthCheck {
@ApiResponse(code = 200, message = "Broker is healthy"),
@ApiResponse(code = 503, message = "Broker is not healthy")
})
- public String getBrokerHealth() {
+ public String getBrokerHealth(@QueryParam("serverInstance") String
serverInstance) {
Review Comment:
I feel we should not overload the broker health check API.
Consider adding a new API just for server to ask if broker has routing for it
##########
pinot-broker/src/main/java/org/apache/pinot/broker/routing/manager/BaseBrokerRoutingManager.java:
##########
@@ -1258,6 +1258,19 @@ public Set<String> getServingInstances(String
tableNameWithType) {
return routingEntry._instanceSelector.getServingInstances();
}
+ /// Returns whether the server is currently available to the broker routing
entries.
+ ///
+ /// The read lock ensures this method cannot observe
`_routableServerInstanceMap` while an instance-config callback
+ /// is still applying the corresponding change to individual routing entries.
+ public boolean isServerRoutable(String instanceId) {
+ _globalLock.readLock().lock();
+ try {
+ return _routableServerInstanceMap.containsKey(instanceId);
Review Comment:
Should we check `_enabledServerInstanceMap.containsKey(instanceId)` instead?
We just need to know if broker sees the server as enabled
--
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]
