gortiz commented on PR #19383: URL: https://github.com/apache/pinot/pull/19383#issuecomment-5477095592
Thanks — all three notes are addressed in d37017f. Your `KeepAliveManager` and `WriteMonitoringFrameWriter` readings match what I measured, and note 1 was right in a way I could reproduce. **1. Permit enforcement now has a behavioural test.** I confirmed your finding first: with both `permitKeepAlive*` calls deleted from `GrpcMailboxServer`, all three tests in `MailboxServerPermitKeepAliveTest` still pass. `MailboxServerPermitKeepAliveBehaviorTest` drives two mailbox servers through **one** 30s window with the same client pinging at gRPC's 10s floor — one permitting the rate, one left at Netty's default. The restrictive one must end the stream with `too_many_pings`; the permissive one must keep it. Running both in one window is what makes it ablation-proof, and it is: with the wiring removed the *permitted* assertion fails with ``` RESOURCE_EXHAUSTED: Connection closed after GOAWAY. HTTP/2 error code: ENHANCE_YOUR_CALM (Bandwidth exhausted), debug data: too_many_pings ``` which is the status you measured. 30.5s, no handshake needed — `open` accepts a stream without the mailbox-id header and only reads it when a message arrives, so the connection carries nothing but pings. **2. The config hop is covered.** `MailboxServiceKeepAliveWiringTest` asserts the `ChannelManager` holds exactly `GrpcKeepAliveConfig.forMailboxChannels(config)` for the default, overridden and disabled cases, so passing `DISABLED` there now fails a test. `ChannelManager#getKeepAliveConfig` became public for it: the assertion spans packages, and the record is immutable so it grants no control over the manager. You were also right about the PR text — `MailboxServiceKeepAliveConfigTest` was folded into `GrpcKeepAliveConfigTest` when the config parsing moved onto the record, and I had not updated the description. Fixed. **3. A drop is now in the logs.** `ChannelManager` logs at WARN when a channel leaves `READY`, re-arming gRPC's single-shot `notifyWhenStateChanged` and stopping on shutdown. Verified on the real transition in `MailboxChannelKeepAliveTest`: ``` WARN [ChannelManager] Mailbox channel to localhost:56004 left READY for IDLE; sends to that peer will fail until it reconnects. If keep-alive reported it, the peer stopped answering about one keep-alive interval ago. ``` 216 tests pass across the affected classes; checkstyle, license and spotless are clean on both modules. One thing your note 3 points at that I deliberately left out of scope: there is still no *metric* for this — nothing counts keep-alive-driven transport deaths, on either the mailbox or the dispatch channels, so it is not alertable. I would rather do that as its own PR than widen this one; happy to open it if you agree. -- 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]
