goingforstudying-ctrl opened a new pull request, #2779: URL: https://github.com/apache/datafusion-ballista/pull/2779
Came across #2673 while reading the pipelined reader's pool code (it was raised in the #2670 review and never got a follow-up). The shared side of `DefaultBallistaClientPool` only shrank when a caller discarded a connection, and even then the slot sat in the map until the next eviction pass. With executor churn those dead slots pile up for the life of the process. Worse, a shared connection that stays live but never gets requested again is never closed at all, since the idle timeout only covered the checked-out deque. There's also a race that bugged me: `shared()` pulls the slot out of the map and then connects outside the shard lock, so an eviction pass in between strands the fresh connection in a detached slot. It still works, but the next caller opens another connection instead of reusing it. What changed: `SharedClient` has an optional on-discard hook now, run on the first discard only. The pool installs one that drops the slot with `remove_if`, but only when the discard came from the connection the slot still holds, so a discard that loses the race to a reconnect can't kill the fresh connection. Eviction also retires live shared connections that haven't been handed out within `idle_timeout` (the executor's `client_ttl`, 30s by default, the same knob the checked-out pool already uses). In-flight streams are fine since they share the tonic channel; this only changes when the pool itself lets go. And after connecting, `shared()` re-attaches its slot if eviction removed it mid-connect, deferring to a different slot that already reconnected. One deliberate deviation from the issue: it proposes `Arc::strong_count == 1` as the no-holders signal, but the pipelined reader drops the `Arc<SharedClient>` as soon as the stream is open (`open_remote_once` returns the stream, not the Arc), so the count sits at 1 while the connection is actively streaming. Went with last-handout time instead, same signal the idle deque uses. `cargo test -p ballista-core -p ballista-executor --lib client_pool` is green (18 tests, 6 new covering eager removal on discard, stale-but-live eviction with a fresh handout surviving, failed-connect slot cleanup, and both ends of the mid-connect eviction race). `cargo clippy --all-targets -- -D warnings` and `cargo fmt --all -- --check` are clean on both crates. Closes #2673. -- 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]
