Kreinoee commented on PR #1316: URL: https://github.com/apache/pekko-http/pull/1316#issuecomment-5841351558
I looked into your comments now. Some I implemented as you suggested, and on some I pushed back a little: **Scope:** understood, 2.0.0 it is. The `@since` tags are back to `2.0.0`. **1. Unbounded drain.** Added `max-connection-age-grace`, passed from the `MaxConnectionAge` timer, with a finite default of **30s**. The value is a plain judgment call (longer than the 20s `request-timeout` that bounds ordinary requests, short enough that one stream cannot hold up rebalancing for long), so if you have a better number in mind, I will change it. One deliberate deviation from your suggestion: `infinite` is still an accepted value, parsed like `max-connection-age`, so the widened `triggerTermination(deadline: Duration)` stays. "Never cut a live stream just to rebalance" is a policy some deployments want, mine included. So if `infinite` is not allowed, we would set it to an arbitrarily high value, which in practice means the same thing, and I think it is better to be able to say so explicitly. Also, you pointed at grpc-java yourself, but there the default actually is infinite ([NettyServerBuilder](https://github.com/grpc/grpc-java/blob/v1.75.0/netty/src/main/java/io/grpc/netty/NettyServerBuilder.java#L112-L113)). With the fix for 2 below, an infinite grace is still bounded by the binding's own `terminate` deadline at shutdown. If you would rather have the grace strictly finite, say so and we will just have to set it to a very high value in our setup. **2. `terminate` on a draining connection.** I made `MaxConnectionAge` guarded against a termination that is already in progress, and made `triggerTermination` reschedule the forced close if the deadline it received is earlier than an already scheduled forced close. In practice that makes it work like this: * `MaxConnectionAge` has no effect if a termination is already in progress * calling `terminate` after `MaxConnectionAge` has fired only reschedules the forced close timer if its deadline is earlier than the one already scheduled by the `MaxConnectionAge` logic, or if none was scheduled because the grace is infinite. I figured that this behaviour is easy to document, and therefore easy for users to reason about: the documentation of `max-connection-age` just states that it has no effect if a termination is already in progress. The alternative I could think of is the "the connection closes no later than the earliest deadline anyone asked for" semantic, but I believe that would be harder for users to reason about. If you prefer it, or a different solution, then let me know. **3. Java API round trip.** Added `JavaDurationConverter.toScala`, the inverse of the existing `toJava`, mapping `ChronoUnit.FOREVER.getDuration` back to `Duration.Inf`. `withMaxConnectionAge` and the new `withMaxConnectionAgeGrace` go through it, and a new `Http2ServerSettingsSpec` has the round-trip tests. Note that the existing Java setters for the other possibly-infinite settings (`ConnectionPoolSettings.withIdleTimeout` / `withKeepAliveTimeout` / `withMaxConnectionLifetime`, `ClientConnectionSettings.withIdleTimeout`, `WebSocketSettings.withPeriodicKeepAliveMaxIdle`) have the same problem, since `toScala` throws on that value too. I left them out of this PR; I can open a follow-up switching them to the same helper once this is merged. **Minor:** both done. The HTTP/2-only note is in reference.conf and the docs, and the test comment now says what the assertion checks rather than tightening the margin. **One extra:** while running the grace period against a real grpc-java client I noticed that the server-side forced-close log message pointed at `completion-timeout`, which is a client-only setting. I made the message side-aware, so on the server it names the `terminate` deadline and `max-connection-age-grace` instead. Shout if you would rather keep that out of this PR. Verification after the changes: http2-tests 380/380 (4 new tests), `+http-core/mimaReportBinaryIssues` clean on 2.13 and 3 (7 filters now), `checkCodeStyle`, `validatePullRequest` and the docs build all pass. The grpc-java 1.75.0 probe from the description was rerun with `max-connection-age = 5s`: with `max-connection-age-grace = 2s`, 77 calls, 0 failures, and a call that never completes server-side failed 3.7 s after it started with `UNAVAILABLE: Connection closed after GOAWAY`; with `max-connection-age-grace = infinite`, again 0 failures, and the same call was kept open by the server until the client's 10 s deadline ended it. -- 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]
