andygrove opened a new pull request, #2236: URL: https://github.com/apache/datafusion-ballista/pull/2236
# Which issue does this PR close? Backport of #2225 to `branch-54`. The issue it fixes is #2224. # Rationale for this change In push-based scheduling the executor tells the scheduler about itself before its own gRPC server is listening, and the scheduler dials that port back to verify connectivity. When the callback wins the race, the executor dies at startup. The ordering in `executor_server::startup` is: 1. `tokio::spawn` the tonic server. `Server::serve` binds *inside* the future, so the socket does not exist yet when `spawn` returns, even though the `"... Grpc Server listening on ..."` line is already in the log. 2. `register_executor`, immediately. On the scheduler side `ExecutorManager::register_executor` calls `test_connectivity`, a single `ExecutorGrpcClient::connect(...)` with no retry and no backoff. If the spawned task has not reached its `bind` yet, that connect gets `ECONNREFUSED`, registration returns an error, and `executor_process` treats a registration error as fatal, so the executor exits instead of joining the cluster. The opposite direction is already tolerant: the executor retries its connection *to* the scheduler in a loop. Only the scheduler's callback to the executor is one-shot, which is why the executor's own startup ordering has to be right. It is timing-dependent, so it shows up rarely and on loaded machines. Outside of tests the consequence is an otherwise healthy executor failing to join, and in a restart loop it can keep failing to join. # What changes are included in this PR? A clean cherry-pick of 984513ee, unmodified. - `ballista/core/src/utils.rs` — new `create_grpc_server_incoming(addr, &GrpcServerConfig)`, which binds the listening socket eagerly and returns tonic's `TcpIncoming`. tonic ignores the server builder's `tcp_nodelay` / `tcp_keepalive` when serving from a pre-bound listener, so the helper applies the same values `create_grpc_server` sets and keeps the two in one place. The remaining settings still come from the builder as before, so the socket is configured exactly as it was. - `ballista/executor/src/executor_server.rs` — `startup` binds via that helper on the current task, before spawning, and the spawned task serves with `serve_with_incoming_shutdown`. Registration therefore cannot run before the port is accepting connections. One incidental improvement falls out of binding eagerly: a port conflict now fails `startup` directly, with the bind error, rather than being discovered later through the spawned server task. New tests in `ballista-core`: - `test_create_grpc_server_incoming_binds_eagerly` — connects to the bound address while nothing is serving on it. This is the property the fix depends on, and it fails against a lazily-bound socket. - `test_create_grpc_server_incoming_port_in_use` — binding an address twice is an error rather than a panic. # Are there any user-facing changes? No behaviour changes for a successfully started executor and no API breakage. `create_grpc_server_incoming` is a new public function in `ballista-core`, additive alongside `create_grpc_server`. It must be called from within a Tokio runtime, since the listener registers with the reactor; this is documented on the function. --- Verified locally on the `branch-54` base: `cargo fmt --all -- --check` is clean, and `cargo check --workspace --all-targets --locked` completes with no warnings on a combined stack of the six backports being proposed together. Test execution is left to CI. -- 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]
