jineshparakh commented on PR #19407: URL: https://github.com/apache/pinot/pull/19407#issuecomment-5510545020
Thanks for the review — it was unusually useful, and two of the items turned out to be load-bearing rather than cosmetic. All six inline comments are addressed in `37ad9142`; replies are on the individual threads. Here are the three you raised at the top level. ### On the test gap You were right, and it would have passed with the feature stubbed out. Worse than you spotted: `registerServiceStatusHandler()` computes `resourcesToMonitor` from the `brokerResource` ideal state at registration time, and with no table added yet this broker is in no partition — so the Helix callbacks report `GOOD` immediately and `preConnect()` returns 0 on an empty server map. `startServer()` now precedes `startBroker()`, and the assertions are stronger: one channel per (server, table type), plus a slow-connector case proving readiness is released within the budget. Separately, the TLS path had no coverage at all, which is worse given that TLS is the reason the change exists. Added `TlsIntegrationTest#testPreConnectOpensTlsChannelsToEveryServer` — that cluster runs its server with `netty.enabled=false` / `nettytls.enabled=true`, so every single-stage channel the broker opens carries a real `SslHandler`. Pre-connect counts a channel as connected only once the handshake completes, so asserting the channel count there is what proves the handshake is actually awaited. Without it the TLS path could break silently, since pre-connect swallows its own failures by design and falls back to the lazy path — every other assertion in that class would still pass. ### 1. Is the readiness gate worth it? I've kept it, but the mechanism is now much smaller, which I think changes the trade. It no longer adds anything: no new status value, no extra `ServiceStatusCallback`, no separate completion flag. Pre-connect is part of startup, so it simply keeps `_isStarting` set until it finishes and the `LifecycleServiceStatusCallback` that is already registered reports `STARTING` as it always would. That also removes the ordering hazard the first version needed a comment to explain away — `_isStarting` is set before the status handler is registered, so there is structurally no un-gated window rather than one avoided by care. `BaseBrokerStarter` is a net deletion as a result. On the probe risk you are right that the broker has only `/health`, with no liveness/readiness split. One correction though: the bundled chart does **not** define a startupProbe for the broker — `values.yaml` has `startupEnabled: false` with `livenessEnabled: true` pointed at `/health`. So the safety there comes from `failureThreshold: 10 x periodSeconds: 10` = 100s against a 30s budget, not from a startupProbe. That makes your underlying concern stronger, not weaker. The real fix is to give the broker `/health/liveness` and `/health/readiness`, mirroring what `pinot-server` has had for a long time (`HealthCheckResource`). I prototyped it and then took it back out: it is a pre-existing gap that any future readiness gate hits, and bundling an endpoint addition into this change is the wrong trade. I'd rather do it as its own PR — happy to open one. ### 2. Is one-shot warm-up the right scope? Agreed on all three limitations, and none of them are addressed here. One correction on the mechanism for full-cluster cold start: `getRoutableServerInstanceMap()` is built from persistent instance configs, not live instances, so the servers *are* in the map and the connects fail fast — rather than the map being empty. The outcome is the one you called, though: `connected == 0` and the step achieves nothing. Scale-up and later-joining servers are likewise uncovered. Hooking routing changes, or moving channel establishment out of the `sendRequest` lock into a shared per-channel future, would cover all three. I'd like to keep those separate — the second in particular is a hot-path change, and this PR's measured win is specifically the startup case. ### On the metric name Fixed, though not as `NETTY_CONNECTION_CONNECT_TIME_MS`. Both enums derive their metric name from the constant via `Utils.toCamelCase(name().toLowerCase())`, so that name renders identically to the existing `BrokerGauge.NETTY_CONNECTION_CONNECT_TIME_MS` and the two collide in the metrics registry — `YammerSettableGauge cannot be cast to Timer`, which failed three tests the moment I made the change you asked for. Renamed to `NETTY_CONNECTION_CONNECT_LATENCY_MS`: carries the `_MS` suffix, matches `NETTY_CONNECTION_SEND_REQUEST_LATENCY` next door, and is no longer confusable with the gauge. The timer is new in this PR, so nothing published is being renamed. Your nit was more than a nit — thanks. -- 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]
