villebro commented on PR #2448: URL: https://github.com/apache/datafusion-ballista/pull/2448#issuecomment-5877244088
> Would iy make sense / is it possible to prevent new constructions on termination ? @milenkovicm lol we seem to be thinking exactly alike on this topic 😆 I actually already looked into this prior to posting my comment (maybe I should have added a quick note on this). Basically this is already kinda covered, just later than SIGTERM. Once shutdown reaches the point where `notify_shutdown` fires, tonic's `serve_with_shutdown` sends an HTTP/2 GOAWAY on every open connection, which refuses new streams and new connections from that point on. So nothing can keep piling new requests onto the drain once it's actually started. Between SIGTERM and that point (heartbeat + `executor_stopped` RPC + task draining), new connections can still land, but I don't think we should close that gap: results are peer-to-peer with no redundancy, so a client with a ticket for this executor has nowhere else to fetch from. Cordoning earlier than we already do would just turn "client waits a bit and succeeds" into "client's connection is refused outright," which is the same failure this PR fixes, just moved earlier instead of removed. -- 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]
