Skip to content

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

Development

Successfully merging this pull request may close these issues.

4 participants