goutamadwant commented on PR #19178: URL: https://github.com/apache/pinot/pull/19178#issuecomment-5842725639
> I found three readiness issues in the current revision ([63432fd](https://github.com/apache/pinot/commit/63432fdd875b8cc49f003e4880b56f27b5cc7811)): > > 1. **An enabled server can be reported as disabled.** In [`BaseBrokerRoutingManager.isServerEnabled()`](https://github.com/apache/pinot/blob/63432fdd875b8cc49f003e4880b56f27b5cc7811/pinot-broker/src/main/java/org/apache/pinot/broker/routing/manager/BaseBrokerRoutingManager.java#L1365), `_instanceConfigChangeInProgress` makes the result false for _every_ server throughout any instance-config refresh. An already-enabled server therefore receives a 503 from the broker routing endpoint while an unrelated server's config is being processed, even though it remains routable. That false negative can delay its startup readiness check. The flag is global rather than tied to the server whose routing changed. > 2. **The existing service-status gate is lost.** [`HealthCheckResource`](https://github.com/apache/pinot/blob/63432fdd875b8cc49f003e4880b56f27b5cc7811/pinot-server/src/main/java/org/apache/pinot/server/api/resources/HealthCheckResource.java#L117) now uses the supplied BooleanSupplier without also requiring `ServiceStatus.GOOD`. With the default non-exiting behavior, startup can time out while status is still `STARTING`, set the query-ready flag, and return 200 from `/health` and `/health/readiness`. The [updated test](https://github.com/apache/pinot/blob/63432fdd875b8cc49f003e4880b56f27b5cc7811/pinot-server/src/test/java/org/apache/pinot/server/api/HealthCheckResourceTest.java#L62) expects 200 even when the service-status callback is `BAD`. > 3. **The fail-open deadline can be exceeded.** [`BrokerRoutingReadyChecker`](https://github.com/apache/pinot/blob/63432fdd875b8cc49f003e4880b56f27b5cc7811/pinot-server/src/main/java/org/apache/pinot/server/starter/helix/BrokerRoutingReadyChecker.java#L206) probes brokers sequentially and checks the deadline only after the sweep. Multiple slow successful probes followed by a failed probe can keep the server unready substantially longer than the configured timeout when fail-open is enabled. @Jackie-Jiang Thanks for the review. I pushed an update addressing all three findings: 1. Replaced the global instance-config-in-progress flag with per-server pending routing publication. An unrelated server update no longer makes an already-routable server appear disabled. If publication fails, it is retried with bounded exponential backoff and jitter, and the affected server is acknowledged only after the routing update succeeds. 2. Restored the `ServiceStatus.GOOD` requirement, so `/health` and `/health/readiness` cannot return 200 while the server is still `STARTING` or otherwise not in a good service state. 3. Enforced the fail-open deadline before and between broker probes, as well as while a background check is still in progress, so sequential probes cannot keep readiness false beyond the configured deadline. I also capped broker readiness response bodies and immediately abort oversized or incomplete responses, and ensured that both local and remote routing-manager shutdown paths stop the retry executor. Validation on JDK 25: - `HttpClientTest`: 3 passed - `BrokerRoutingManagerTest`, `HelixBrokerStarterTest`, and `RemoteClusterBrokerRoutingManagerTest`: 38 passed - `BaseServerStarterTest`, `BrokerRoutingReadyCheckerTest`, and `HealthCheckResourceTest`: 18 passed - Spotless, Checkstyle, and license checks passed for the affected modules The update is in the latest push. Could you please take another look and let me know ? -- 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]
