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-43342][K8S] Revert SPARK-39006 Show a directional error message for executor PVC dynamic allocation failure #41069

Closed
wants to merge 1 commit into from

Conversation

dongjoon-hyun
Copy link
Member

@dongjoon-hyun dongjoon-hyun commented May 5, 2023

What changes were proposed in this pull request?

This reverts commit b065c94.

Why are the changes needed?

To remove the regression from SPARK-39006.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Pass the CIs.

Closes #41057

…tor PVC dynamic allocation failure"

This reverts commit b065c94.
@dongjoon-hyun
Copy link
Member Author

cc @dcoliversun This will be merged with your authorship.

@dongjoon-hyun
Copy link
Member Author

After merging this clean-revert, you may want to make a new and proper test PR for https://github.com/apache/spark/pull/41057/files#r1186469992 if you want.

@dcoliversun
Copy link
Contributor

@dongjoon-hyun Thanks for your PR. I'm +1

@dongjoon-hyun
Copy link
Member Author

Thank you, @dcoliversun .

@dongjoon-hyun
Copy link
Member Author

Could you review this PR, @viirya ?

@viirya
Copy link
Member

viirya commented May 6, 2023

May I ask why this is reverted?

@dongjoon-hyun
Copy link
Member Author

dongjoon-hyun commented May 7, 2023

Sorry for missing context, @viirya .

SPARK-43329 and SPARK-43342 were filed as a regression at Apache Spark 3.4.0 and those issues have the error message, IllegalArgumentException: PVC ClaimName: a1pvc should contain OnDemand or SPARK_EXECUTOR_ID when requiring multiple executors.

I linked all the reported issues to the original JIRA which caused this regression.

Screenshot 2023-05-06 at 5 13 51 PM

And, the following was the reverting PR from the author of SPARK-39006, @dcoliversun.

In short, while adding a log message, we missed a corner case which one NFS-backed PVC is mounted by multiple executors. So, we decide to revert SPARK-39006 because it doesn't give actual benefit (except logging).

@viirya
Copy link
Member

viirya commented May 7, 2023

Thank you @dongjoon-hyun

@dongjoon-hyun
Copy link
Member Author

Thank you!

dongjoon-hyun pushed a commit that referenced this pull request May 7, 2023
…e for executor PVC dynamic allocation failure

### What changes were proposed in this pull request?

This reverts commit b065c94.

### Why are the changes needed?

To remove the regression from SPARK-39006.

### Does this PR introduce _any_ user-facing change?

No.

### How was this patch tested?

Pass the CIs.

Closes #41057

Closes #41069 from dongjoon-hyun/SPARK-43342.

Authored-by: Qian.Sun <[email protected]>
Signed-off-by: Dongjoon Hyun <[email protected]>
(cherry picked from commit 3ba1fa3)
Signed-off-by: Dongjoon Hyun <[email protected]>
@dongjoon-hyun
Copy link
Member Author

Merged to master/3.4.

@dongjoon-hyun dongjoon-hyun deleted the SPARK-43342 branch May 7, 2023 01:28
LuciferYang pushed a commit to LuciferYang/spark that referenced this pull request May 10, 2023
…e for executor PVC dynamic allocation failure

### What changes were proposed in this pull request?

This reverts commit b065c94.

### Why are the changes needed?

To remove the regression from SPARK-39006.

### Does this PR introduce _any_ user-facing change?

No.

### How was this patch tested?

Pass the CIs.

Closes apache#41057

Closes apache#41069 from dongjoon-hyun/SPARK-43342.

Authored-by: Qian.Sun <[email protected]>
Signed-off-by: Dongjoon Hyun <[email protected]>
snmvaughan pushed a commit to snmvaughan/spark that referenced this pull request Jun 20, 2023
…e for executor PVC dynamic allocation failure

### What changes were proposed in this pull request?

This reverts commit b065c94.

### Why are the changes needed?

To remove the regression from SPARK-39006.

### Does this PR introduce _any_ user-facing change?

No.

### How was this patch tested?

Pass the CIs.

Closes apache#41057

Closes apache#41069 from dongjoon-hyun/SPARK-43342.

Authored-by: Qian.Sun <[email protected]>
Signed-off-by: Dongjoon Hyun <[email protected]>
(cherry picked from commit 3ba1fa3)
Signed-off-by: Dongjoon Hyun <[email protected]>
GladwinLee pushed a commit to lyft/spark that referenced this pull request Oct 10, 2023
…e for executor PVC dynamic allocation failure

### What changes were proposed in this pull request?

This reverts commit b065c94.

### Why are the changes needed?

To remove the regression from SPARK-39006.

### Does this PR introduce _any_ user-facing change?

No.

### How was this patch tested?

Pass the CIs.

Closes apache#41057

Closes apache#41069 from dongjoon-hyun/SPARK-43342.

Authored-by: Qian.Sun <[email protected]>
Signed-off-by: Dongjoon Hyun <[email protected]>
(cherry picked from commit 3ba1fa3)
Signed-off-by: Dongjoon Hyun <[email protected]>
catalinii pushed a commit to lyft/spark that referenced this pull request Oct 10, 2023
…e for executor PVC dynamic allocation failure

### What changes were proposed in this pull request?

This reverts commit b065c94.

### Why are the changes needed?

To remove the regression from SPARK-39006.

### Does this PR introduce _any_ user-facing change?

No.

### How was this patch tested?

Pass the CIs.

Closes apache#41057

Closes apache#41069 from dongjoon-hyun/SPARK-43342.

Authored-by: Qian.Sun <[email protected]>
Signed-off-by: Dongjoon Hyun <[email protected]>
(cherry picked from commit 3ba1fa3)
Signed-off-by: Dongjoon Hyun <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants