pjfanning commented on PR #1316: URL: https://github.com/apache/pekko-http/pull/1316#issuecomment-5887566762
Thanks @Kreinoee for the thorough follow-up. All the points from the earlier review are addressed, and the explanations were really helpful. Keeping `infinite` as an allowed grace value is fine by me, since with the fix for the binding-level `terminate` it can't hold up shutdown. The side-aware forced-close log message is a nice extra, and a follow-up PR for the other Java setters with the same `FOREVER` round-trip problem would be welcome. A few small points, none of them blocking: - `reference.conf`: the `max-connection-age` comment says an age expiry has no effect on a termination already in progress, but not the converse (a later `terminate` with an earlier deadline shortens a drain the age started). `http2.md` covers both, so it would be good to mirror that in `reference.conf`. - Test coverage: there's no test for `max-connection-age-grace = infinite` followed by a later `terminate(short)`. The code handles it (no forced close is scheduled, so the new deadline always wins), but since that combination is what justifies allowing `infinite`, a test pinning it down would be worthwhile. - Some of the new tests have fairly tight timing margins (e.g. `expectNoBytes(400.millis)` against a 500ms age, `expectNoBytes(700.millis)` against a 1s terminate). They're probably fine; if any of them turn out flaky on CI, widening the margins would be the first thing to try. -- 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]
