andygrove commented on PR #2242: URL: https://github.com/apache/datafusion-ballista/pull/2242#issuecomment-5205765717
Thanks for tracking this down, the diagnosis matches what I found in #2240. One thing to watch out for: `unassigned_executor_slots` is not quite the same set as "executors we removed". `launch_tasks` also pushes a slot when `get_executor_metadata` fails, and that branch never calls `remove_executor`, so the executor is still registered and probably still healthy. Posting `ExecutorLost` for it makes the handler run `reset_stages_on_lost_executor`, which rolls back completed stages whose shuffle output lives there and cancels its running tasks. A transient cluster-state read error would then throw away finished work, and because the rollback triggers `ReviveOffers` it can loop. I think it is safer to post the event at the point where the executor is actually removed rather than inferring it from the slot list. That also covers the case where a launch task panics and `join_all` returns `Err` after the removal already happened. Second thing: could you add a test? The failure is an intermittent hang so there is nothing keeping it fixed otherwise. I added `lose_executor_on_launch_failure` to the scheduler test harness in #2240 which makes this deterministic, feel free to take it. There is some overlap between this and #2240, happy either way on which one we land. -- 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]
