Skip to content

Conversation

@iRakson
Copy link
Contributor

@iRakson iRakson commented Apr 18, 2020

What changes were proposed in this pull request?

A similar patch is tested here #26901

Why are the changes needed?

When executors do not goes down gracefully, their stop() method is never called. To solve this problem, a shutdown hook was used to execute the stop() method.
Instead of forcing a shutdown hook, we should just add a StopExecutor call to executors before they are killed.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Manually

@iRakson
Copy link
Contributor Author

iRakson commented Apr 18, 2020

cc @dongjoon-hyun @vanzin
Kindly take a look.

@dongjoon-hyun
Copy link
Member

Since this PR aims reverts the original patch of SPARK-29152 and add a new way, I believe @vanzin 's option is important to this.

@dongjoon-hyun
Copy link
Member

ok to test

@SparkQA
Copy link

SparkQA commented Apr 18, 2020

Test build #121459 has finished for PR 28254 at commit c4d3711.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

val killExecutors: Boolean => Future[Boolean] =
if (executorsToKill.nonEmpty) {
executorsToKill.foreach(id =>
executorDataMap.get(id).foreach(_.executorEndpoint.send(StopExecutor)))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we guarantee that stop is called before kill in this way?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

StopExecutor may arrive at executor after kill arrive at worker/container due to network delay, isn't it possible?

@vanzin
Copy link
Contributor

vanzin commented Apr 22, 2020

Sorry, don't really have the cycles for a detailed review. But if you remove the shutdown hook, you'll be missing certain executor exit scenarios, like a cluster manager killing the executor.

@iRakson
Copy link
Contributor Author

iRakson commented Jun 14, 2020

For the time being we are continuing with the current approach only. Closing this PR.

@iRakson iRakson closed this Jun 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants