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-48292][CORE] Revert [SPARK-39195][SQL] Spark OutputCommitCoordinator should abort stage when committed file not consistent with task status #46696

Closed
wants to merge 2 commits into from

Conversation

AngersZhuuuu
Copy link
Contributor

@AngersZhuuuu AngersZhuuuu commented May 22, 2024

What changes were proposed in this pull request?

Revert #36564 According to discuss #36564 (comment)

When spark commit task will commit to committedTaskPath
${outputpath}/_temporary//${appAttempId}/${taskId}
So in #36564 's case, since before #38980, each task's job id's date is not the same, when the task writes data success but fails to send back TaskSuccess RPC, the task rerun will commit to a different committedTaskPath then causing data duplicated.

After #38980, for the same task's different attempts, the TaskId is the same now, when re-run task commit, will commit to the same committedTaskPath, and hadoop CommitProtocol will handle such case then data won't be duplicated.

Note: The taskAttemptPath is not same since in the path contains the taskAttemptId.

Why are the changes needed?

No need anymore

Does this PR introduce any user-facing change?

No

How was this patch tested?

Existed UT

Was this patch authored or co-authored using generative AI tooling?

No

…nator should abort stage when committed file not consistent with task status
@AngersZhuuuu
Copy link
Contributor Author

ping @cloud-fan

@github-actions github-actions bot added the CORE label May 22, 2024
// Regression test for SPARK-10381
val e = intercept[SparkException] {
failAfter(Span(60, Seconds)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

shall we still check the error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Won't throw error after revert...., it can run success.

@cloud-fan
Copy link
Contributor

Can we explain this a bit more about why the issue is gone now?
image

@AngersZhuuuu
Copy link
Contributor Author

Can we explain this a bit more about why the issue is gone now? image

Added to pr desc

@cloud-fan
Copy link
Contributor

will commit to the same committedTaskPath, and hadoop CommitProtocol will handle such case then data won't be duplicated.

Will we hit file already exist exception in this case?

@AngersZhuuuu
Copy link
Contributor Author

will commit to the same committedTaskPath, and hadoop CommitProtocol will handle such case then data won't be duplicated.

Will we hit file already exist exception in this case?

commitTask will overwrite the existed committedTaskPath , won't throw file already exception.
截屏2024-05-24 10 19 10

@cloud-fan
Copy link
Contributor

can we also revert #46562 in this PR?

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.

+1, LGTM. Also, +1 to include the additional revert into here.

cc @viirya

@viirya
Copy link
Member

viirya commented May 24, 2024

Looks good to me.

@AngersZhuuuu AngersZhuuuu changed the title [SPARK-48292[CORE] Revert [SPARK-39195][SQL] Spark OutputCommitCoordinator should abort stage when committed file not consistent with task status [SPARK-48292][CORE] Revert [SPARK-39195][SQL] Spark OutputCommitCoordinator should abort stage when committed file not consistent with task status May 29, 2024
@AngersZhuuuu
Copy link
Contributor Author

can we also revert #46562 in this PR?

Done

@github-actions github-actions bot added the SQL label May 29, 2024
@AngersZhuuuu
Copy link
Contributor Author

GA passed cc @cloud-fan

@cloud-fan
Copy link
Contributor

thanks, merging to master!

@cloud-fan cloud-fan closed this in f68d761 May 30, 2024
riyaverm-db pushed a commit to riyaverm-db/spark that referenced this pull request Jun 7, 2024
…inator should abort stage when committed file not consistent with task status

### What changes were proposed in this pull request?
Revert apache#36564 According to discuss apache#36564 (comment)

When spark commit task will commit to committedTaskPath
`${outputpath}/_temporary//${appAttempId}/${taskId}`
So in apache#36564 's case, since before apache#38980, each task's job id's date is not the same,  when the task writes data success but fails to send back TaskSuccess RPC, the task rerun will commit to a different committedTaskPath then causing data duplicated.

After apache#38980, for the same task's different attempts, the TaskId is the same now, when re-run task commit, will commit to the same committedTaskPath, and hadoop CommitProtocol will handle such case then data won't be duplicated.

Note: The taskAttemptPath is not same since in the path contains the taskAttemptId.

### Why are the changes needed?
No need anymore

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

### How was this patch tested?
Existed UT

### Was this patch authored or co-authored using generative AI tooling?
No

Closes apache#46696 from AngersZhuuuu/SPARK-48292.

Authored-by: Angerszhuuuu <angers.zhu@gmail.com>
Signed-off-by: Wenchen Fan <wenchen@databricks.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants