Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revert "[SPARK-35011][CORE] Avoid Block Manager registrations when StopExecutor msg is in-flight" #33942

Closed
wants to merge 1 commit into from

Conversation

Ngone51
Copy link
Member

@Ngone51 Ngone51 commented Sep 9, 2021

This reverts commit b9e53f8.

What changes were proposed in this pull request?

Revert #32114

Why are the changes needed?

It breaks the expected BlockManager re-registration (e.g., heartbeat loss of an active executor) due to deferred removal of BlockManager, see the check:

Does this PR introduce any user-facing change?

No

How was this patch tested?

Pass existing tests.

…opExecutor msg is in-flight"

This reverts commit b9e53f8.
@github-actions github-actions bot added the CORE label Sep 9, 2021
@Ngone51
Copy link
Member Author

Ngone51 commented Sep 9, 2021

@Ngone51
Copy link
Member Author

Ngone51 commented Sep 9, 2021

The original PR has been merged into other branches(3.2, 3.1, 3.0) We should revert them as well.

@SparkQA
Copy link

SparkQA commented Sep 9, 2021

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/47609/

@SparkQA
Copy link

SparkQA commented Sep 9, 2021

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/47609/

@SparkQA
Copy link

SparkQA commented Sep 9, 2021

Test build #143105 has finished for PR 33942 at commit dcddf9f.

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

@dongjoon-hyun
Copy link
Member

Thank you for pinging me, @Ngone51 .

@dongjoon-hyun
Copy link
Member

cc @viirya , @sunchao for FYI

@mridulm
Copy link
Contributor

mridulm commented Sep 10, 2021

@dongjoon-hyun, you had asked about adding a test case to check for this (I think I saw that in the mail, but cant find that comment here) - that is an excellent idea, and we should test for it.
Can be a follow up PR in master ofcourse !

@dongjoon-hyun
Copy link
Member

Ya, I wrote it and took back my comment, @mridulm . :)

@dongjoon-hyun
Copy link
Member

Could you make reverting PRs to the old branches please, @Ngone51 ? We had better pass CIs at those branches.

@Ngone51
Copy link
Member Author

Ngone51 commented Sep 10, 2021

Sure, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
4 participants