goutamadwant commented on code in PR #19178:
URL: https://github.com/apache/pinot/pull/19178#discussion_r4033181951
##########
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:
Addressed. The broker now exposes a dedicated `GET
/routing/server/{instanceId}` endpoint; broker health remains unchanged.
##########
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:
Addressed. The endpoint no longer acquires the routing lock. It reads the
enabled-server map plus a completion publication flag, so it cannot acknowledge
a newly enabled server while table routing entries are still being updated.
##########
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:
Addressed. `isServerEnabled()` now checks
`_enabledServerInstanceMap.containsKey(instanceId)`. The additional completion
flag only prevents acknowledgement during the instance-config routing update
window.
##########
pinot-server/src/main/java/org/apache/pinot/server/api/resources/HealthCheckResource.java:
##########
@@ -55,6 +56,10 @@ public class HealthCheckResource {
@Inject
private AtomicBoolean _shutDownInProgress;
Review Comment:
Addressed. Both readiness and shutdown state use named `BooleanSupplier`
injection, avoiding unnamed mutable-type injection.
##########
pinot-server/src/main/java/org/apache/pinot/server/api/resources/HealthCheckResource.java:
##########
@@ -111,6 +116,11 @@ public String checkReadiness() {
throw new WebApplicationException(errMessage,
Response.status(Response.Status.SERVICE_UNAVAILABLE).entity(errMessage).build());
}
+ if (!_isServerReadyToServeQueries.getAsBoolean()) {
+ String errMessage = "Server is not ready to serve queries";
+ throw new WebApplicationException(errMessage,
+
Response.status(Response.Status.SERVICE_UNAVAILABLE).entity(errMessage).build());
+ }
Status status = ServiceStatus.getServiceStatus(_instanceId);
Review Comment:
Addressed. When the server-readiness supplier is present, the health
resource uses it directly and does not repeat the `ServiceStatus` readiness
check.
##########
pinot-server/src/main/java/org/apache/pinot/server/api/resources/HealthCheckResource.java:
##########
@@ -55,6 +56,10 @@ public class HealthCheckResource {
@Inject
private AtomicBoolean _shutDownInProgress;
+ @Inject
+ @Named(AdminApiApplication.SERVER_READY_TO_SERVE_QUERIES)
+ private BooleanSupplier _isServerReadyToServeQueries;
Review Comment:
Addressed. The field is named `_serverReadyToServeQueries`.
##########
pinot-server/src/main/java/org/apache/pinot/server/api/AdminApiApplication.java:
##########
@@ -63,8 +65,8 @@ public class AdminApiApplication extends ResourceConfig {
public AdminApiApplication(ServerInstance instance, AccessControlFactory
accessControlFactory,
- ServerReloadJobStatusCache reloadJobStatusCache,
- PinotConfiguration serverConf) {
+ ServerReloadJobStatusCache reloadJobStatusCache, PinotConfiguration
serverConf,
+ BooleanSupplier isServerReadyToServeQueries) {
Review Comment:
Addressed. The constructor parameter is named `serverReadyToServeQueries`.
--
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]
