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

[SPARK-40596][CORE] Populate ExecutorDecommission with messages in ExecutorDecommissionInfo #38030

Closed
wants to merge 4 commits into from

Conversation

bozhang2820
Copy link
Contributor

@bozhang2820 bozhang2820 commented Sep 28, 2022

What changes were proposed in this pull request?

This change populates ExecutorDecommission with messages in ExecutorDecommissionInfo.

Why are the changes needed?

Currently the message in ExecutorDecommission is a fixed value ("Executor decommission."), so it is the same for all cases, e.g. spot instance interruptions and auto-scaling down. With this change we can better differentiate those cases.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Added a unit test.

@bozhang2820 bozhang2820 changed the title [SPARK-40596] Populate ExecutorDecommission with messages in ExecutorDecommissionInfo [SPARK-40596][CORE] Populate ExecutorDecommission with messages in ExecutorDecommissionInfo Sep 28, 2022
@github-actions github-actions bot added the CORE label Sep 28, 2022
@mridulm
Copy link
Contributor

mridulm commented Sep 28, 2022

How/where are we using the message ?

@mridulm
Copy link
Contributor

mridulm commented Sep 28, 2022

Ok, so this is mainly to propagate in SparkListenerExecutorRemoved, sounds reasonable.

+CC @dongjoon-hyun

@dongjoon-hyun
Copy link
Member

Thank you, @mridulm .

Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

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

Please revert the change on MapStatus.

@dongjoon-hyun
Copy link
Member

Could you address the review comments, @bozhang2820 ?

@bozhang2820
Copy link
Contributor Author

Could you address the review comments, @bozhang2820 ?

Sorry for the late response. I was on vacation last week. Addressed the review comments.

@bozhang2820
Copy link
Contributor Author

Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

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

Thank you for update, @bozhang2820 . It looks good to me.

What do you think about the Today's patch, @mridulm , @Ngone51 , @xuanyuanking?

Copy link
Member

@Ngone51 Ngone51 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@xuanyuanking xuanyuanking left a comment

Choose a reason for hiding this comment

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

LGTM

@Ngone51 Ngone51 closed this in 4eb0edf Oct 11, 2022
@Ngone51
Copy link
Member

Ngone51 commented Oct 11, 2022

Thanks, merged to master!

@dongjoon-hyun
Copy link
Member

Thank you all! :)

@bozhang2820 bozhang2820 deleted the spark-40596 branch October 11, 2022 11:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
5 participants