NoahKusaba commented on code in PR #2607:
URL:
https://github.com/apache/datafusion-ballista/pull/2607#discussion_r4355711703
##########
ballista/executor/src/executor_process.rs:
##########
@@ -652,11 +652,9 @@ async fn start_executor_process_inner(
// this feature was added to support docker-compose so that we can
have the executor
// wait for the scheduler to start, or at least run for 10 seconds
before failing so
// that docker-compose's restart policy will restart the container.
- let start_time = Instant::now().elapsed().as_secs();
+ let start_time = Instant::now();
let mut x = None;
- while x.is_none()
- && Instant::now().elapsed().as_secs() - start_time <
connect_timeout
- {
+ while x.is_none() && start_time.elapsed() <
Duration::from_secs(connect_timeout) {
Review Comment:
Optional nit: computing the deadline once, `let deadline = start_time +
Duration::from_secs(connect_timeout);` and `while x.is_none() && Instant::now()
< deadline`, reads a little more directly.
Separately, and not for this PR: the deadline is only checked between
attempts, so a single `endpoint.connect()` can still run up to the gRPC
`connect_timeout_seconds` (20s by default) past it when the scheduler host
drops packets. `tokio::time::timeout_at(deadline, endpoint.connect())` would
make the timeout strict. That could be a follow-up.
--
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]
