Return server readiness only after query serving starts

[goutamadwant]

Summary

• keep server readiness endpoints unavailable until the query-serving path is ready
• reuse the server's existing local readiness state, which becomes ready after the query server starts and IS_SHUTDOWN_IN_PROGRESS is cleared
• preserve liveness behavior and add regression coverage for the startup window

Why

During startup, ServiceStatus can become GOOD before the query server is started and before brokers can route queries back to the server. This allowed /health and /health/readiness to return 200 too early during rolling restarts.

The health resource uses the existing local readiness signal instead of reading Helix on every probe.

This addresses item 1 of #16565. The broker external-view watermark described in item 2 remains out of scope.

Testing

• ./mvnw -pl pinot-server -am -Dtest=HealthCheckResourceTest,AccessControlTest -Dsurefire.failIfNoSpecifiedTests=false test
• ./mvnw spotless:apply -pl pinot-server
• ./mvnw license:format -pl pinot-server
• ./mvnw checkstyle:check -pl pinot-server
• ./mvnw license:check -pl pinot-server
• ./mvnw test-compile -pl pinot-server -am -Dmaven.compiler.showDeprecation=true -Dmaven.compiler.showWarnings=true '-Dmaven.compiler.compilerArgs=-Xlint:all'

[codecov-commenter]

Codecov Report

❌ Patch coverage is 41.90476% with 61 lines in your changes missing coverage. Please review.
✅ Project coverage is 66.94%. Comparing base (f7e6bb1) to head (eaa2e98).
⚠️ Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
...erver/starter/helix/BrokerRoutingReadyChecker.java	35.89%	48 Missing and 2 partials ⚠️
.../pinot/server/starter/helix/BaseServerStarter.java	0.00%	11 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #19178      +/-   ##
============================================
+ Coverage     57.70%   66.94%   +9.24%     
- Complexity        7     1423    +1416     
============================================
  Files          2658     3453     +795     
  Lines        158824   218634   +59810     
  Branches      26052    34755    +8703     
============================================
+ Hits          91642   146359   +54717     
- Misses        59411    60579    +1168     
- Partials       7771    11696    +3925     

Flag	Coverage Δ
integration	100.00% <ø> (ø)
integration1	100.00% <ø> (ø)
integration2	0.00% <ø> (ø)
java-25	66.94% <41.90%> (+9.24%)	⬆️
lane-a	100.00% <ø> (ø)
lane-b	0.00% <ø> (ø)
temurin	66.94% <41.90%> (+9.24%)	⬆️
unittests	66.93% <41.90%> (+9.24%)	⬆️
unittests1	57.70% <ø> (+<0.01%)	⬆️
unittests2	39.03% <41.90%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[Jackie-Jiang]

Thanks for the contribution.

Unfortunately this solution is still not good enough. There is a delay for broker to get the server config change callback and add it to the routing.
We need to find a way to ensure broker already add the server to the routing table

[goutamadwant]

Thanks @Jackie-Jiang you are right. The current change only ensures that the query server is running before shutdownInProgress is cleared. It then marks the server ready immediately, while brokers process the instance-config callback asynchronously, so readiness can still return 200 before the server has been added to their routing tables.

My proposed next step is an explicit broker-side acknowledgement or processing watermark after the routing manager finishes applying the instance-config change to all routing entries. The server would remain unready until the relevant live brokers have acknowledged the config version that re-enabled it.

Does that coordination point match the approach you prefer, or should this condition instead be exposed to the rolling-restart orchestrator rather than gating the server health endpoint? Let me know. thanks!

[Jackie-Jiang]

I think the following mechanism could work:

• Server set shutDownInProgress to false
• Server monitor EV of brokerResource, ask each online broker whether it marks the server up
• Server mark itself ready once all brokers are ready

[goutamadwant]

Thanks @Jackie-Jiang. I updated the PR to follow the suggested mechanism:

• After clearing shutDownInProgress, the server reads the online brokers from the brokerResource external view.
• It asks each broker whether the server is currently routable.
• The broker check is synchronized with routing updates, so it cannot return success while the instance-config callback is still being applied.
• Server readiness remains false until every online broker confirms. Broker membership is revalidated before caching the ready state.

The checks run concurrently and stop after readiness is established. I also added unit tests and an HTTP integration test covering the 200 and 503 responses. Let me know if you have any comments. thanks!

[jadami10]

Does this mean an unhealthy broker will also prevent servers from starting up? I'm trying to think if this might introduce a blocking situation where losing brokers and servers at the same time makes it impossible for either to go healthy.

Either way, should this start behind a configuration default to off?

[Jackie-Jiang]

Does this mean an unhealthy broker will also prevent servers from starting up? I'm trying to think if this might introduce a blocking situation where losing brokers and servers at the same time makes it impossible for either to go healthy.

Either way, should this start behind a configuration default to off?

Good point. We should introduce a timeout on broker check, and add a config to decide what's the behavior on broker check failure.
I feel the mechanism is desired and we might want to have it on by default.

[jadami10]

I feel the mechanism is desired and we might want to have it on by default.

agreed. though i think it might be worth it to go out in 2 release (one off/non-blocking, then the next one on)