[SPARK-36772] FinalizeShuffleMerge fails with an exception due to attempt id not matching #34018

zhouyejoe · 2021-09-16T07:41:09Z

What changes were proposed in this pull request?

Remove the appAttemptId from TransportConf, and parsing through SparkEnv.

Why are the changes needed?

Push based shuffle will fail if there are any attemptId set in the SparkConf, as the attemptId is not set correctly in Driver.

Does this PR introduce any user-facing change?

No

How was this patch tested?

Tested within our Yarn cluster. Without this PR, the Driver will fail to finalize the shuffle merge on all the mergers. After the patch, Driver can successfully finalize the shuffle merge and the push based shuffle can work fine.
Also with unit test to verify the attemptId is being set in the BlockStoreClient in Driver.

…o attempt id not matching.

zhouyejoe · 2021-09-16T07:41:24Z

cc @mridulm @Ngone51

gengliangwang · 2021-09-16T12:44:08Z

@zhouyejoe Thanks for the fix.
I will cut 3.2.0 RC3 after this one is merged.

core/src/main/scala/org/apache/spark/SparkEnv.scala

common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/BlockStoreClient.java

…lization

zhouyejoe · 2021-09-17T02:46:38Z

Tested the patch within our own cluster with YARN set up. Push based shuffle works fine in yarn cluster mode, where Yarn will provide default application attempt 1 to the job. Added unit test, leveraging the CSMockExternalBlockManager, started a SparkContext and check whether the application attemptId is being set in BlockStoreClient

core/src/main/scala/org/apache/spark/SparkContext.scala

core/src/test/scala/org/apache/spark/SparkContextSuite.scala

core/src/main/scala/org/apache/spark/SparkEnv.scala

gengliangwang · 2021-09-17T05:36:35Z

@zhouyejoe could you update the PR title and description?

zhouyejoe · 2021-09-17T06:23:02Z

@zhouyejoe could you update the PR title and description?

Updated the PR title and description.

...network-shuffle/src/main/java/org/apache/spark/network/shuffle/ExternalBlockStoreClient.java

core/src/main/scala/org/apache/spark/SparkContext.scala

core/src/test/scala/org/apache/spark/scheduler/CoarseGrainedSchedulerBackendSuite.scala

mridulm · 2021-09-17T16:34:23Z

Discussed with @Ngone51.
@zhouyejoe can we make the following change please:

Move toInt from SparkContext into setAttemptId (which will take it as a String param).
try/catch the Integer.parseInt and log warning in case it is not able to parse attempt id to string.
- If successful, set attempt id to that value - else leave it unchanged (defaulting to -1).
Let us keep the attempt id in BlockStoreClient itself.

Thoughts ?

zhouyejoe · 2021-09-17T17:16:04Z

Discussed with @Ngone51.
@zhouyejoe can we make the following change please:

Move toInt from SparkContext into setAttemptId (which will take it as a String param).

try/catch the Integer.parseInt and log warning in case it is not able to parse attempt id to string.

If successful, set attempt id to that value - else leave it unchanged (defaulting to -1).

Let us keep the attempt id in BlockStoreClient itself.

Thoughts ?

SGTM. Will update accordingly.

Ngone51 · 2021-09-17T17:16:52Z

I'm thinking of the potential compatibility issue in the future by using the int type of appAttemptId inside BlockStoreClinet. Using int might impede the future extension of CMs (e.g., K8s, Standalone, or something new) if they want the arbitrary attempt id to include more info. So I think it's good to keep it as a string. However, since push-based shuffle would require a comparable number value for its functionality, we'd still require the attempt id to at least includes one comparable number value, e.g., (xxx_attempt_id_12). Then, in the case of push-based shuffle, we can just extract the number value into int number, which would be compatible with the current push-based shuffle protocols.
@mridulm @zhouyejoe WDYT?

core/src/main/scala/org/apache/spark/executor/CoarseGrainedExecutorBackend.scala

…nt from Int to String, but add comparableAppAttemptId in ExternalBlockStoreClient

zhouyejoe · 2021-09-18T00:11:44Z

I am also thinking about the same.

Discussed with @Ngone51.
@zhouyejoe can we make the following change please:

Move toInt from SparkContext into setAttemptId (which will take it as a String param).

try/catch the Integer.parseInt and log warning in case it is not able to parse attempt id to string.

If successful, set attempt id to that value - else leave it unchanged (defaulting to -1).

Let us keep the attempt id in BlockStoreClient itself.

Thoughts ?

Update:

Changed the AppAttemptId type in BlockStoreClient from Int to String. Add integer comparableAppAttemptId in ExternalBlockStoreClient, and override the getAppAttemptId and setAppAttemptId there.
Try catch the String to Integer parse in setComparableAppAttemptId.
The default value of comparableAppAttemptId is -1.
Only setAppAttemptId in Driver or Executor when Push based shuffle is enabled.
Minor change in PushBasedExternalClusterManager to return 1 for the TaskScheduler.applicationAttemptId()

zhouyejoe · 2021-09-18T00:17:47Z

BTW, I tested in three modes:

Local mode with Spark Pi example.
Yarn client mode from Spark-shell with a large amount of shuffle.
Yarn cluster mode by launching an actual application with a large amount of shuffle.
cc @mridulm

mridulm

Just couple of minor comments - looks good to me.
+CC @Ngone51

...network-shuffle/src/main/java/org/apache/spark/network/shuffle/ExternalBlockStoreClient.java

core/src/main/scala/org/apache/spark/SparkContext.scala

core/src/main/scala/org/apache/spark/executor/CoarseGrainedExecutorBackend.scala

...network-shuffle/src/main/java/org/apache/spark/network/shuffle/ExternalBlockStoreClient.java

zhouyejoe · 2021-09-18T05:48:09Z

Updated. Last run PR test failed on some unrelated unit test.
Jenkins, test this please.

mridulm · 2021-09-18T05:53:45Z

Ok to test

SparkQA · 2021-09-18T06:41:03Z

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

SparkQA · 2021-09-18T06:49:45Z

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

gengliangwang · 2021-09-18T07:51:18Z

Merging to master/3.2
@zhouyejoe thanks for the work and test!
@Ngone51 @mridulm @venkata91 thanks for the review.

…empt id not matching ### What changes were proposed in this pull request? Remove the appAttemptId from TransportConf, and parsing through SparkEnv. ### Why are the changes needed? Push based shuffle will fail if there are any attemptId set in the SparkConf, as the attemptId is not set correctly in Driver. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? Tested within our Yarn cluster. Without this PR, the Driver will fail to finalize the shuffle merge on all the mergers. After the patch, Driver can successfully finalize the shuffle merge and the push based shuffle can work fine. Also with unit test to verify the attemptId is being set in the BlockStoreClient in Driver. Closes #34018 from zhouyejoe/SPARK-36772. Authored-by: Ye Zhou <yezhou@linkedin.com> Signed-off-by: Gengliang Wang <gengliang@apache.org> (cherry picked from commit cabc36b) Signed-off-by: Gengliang Wang <gengliang@apache.org>

SparkQA · 2021-09-18T08:26:55Z

Test build #143433 has finished for PR 34018 at commit f6e47b8.

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

Ngone51 · 2021-09-18T14:13:08Z

Late lgtm. Thanks @zhouyejoe

mridulm · 2021-09-18T22:49:48Z

Thanks for reviewing and merging it @gengliangwang !

zhouyejoe · 2021-09-19T05:53:54Z

Thanks for review @mridulm @Ngone51 @venkata91 @gengliangwang

…empt id not matching ### What changes were proposed in this pull request? Remove the appAttemptId from TransportConf, and parsing through SparkEnv. ### Why are the changes needed? Push based shuffle will fail if there are any attemptId set in the SparkConf, as the attemptId is not set correctly in Driver. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? Tested within our Yarn cluster. Without this PR, the Driver will fail to finalize the shuffle merge on all the mergers. After the patch, Driver can successfully finalize the shuffle merge and the push based shuffle can work fine. Also with unit test to verify the attemptId is being set in the BlockStoreClient in Driver. Closes apache#34018 from zhouyejoe/SPARK-36772. Authored-by: Ye Zhou <yezhou@linkedin.com> Signed-off-by: Gengliang Wang <gengliang@apache.org> (cherry picked from commit cabc36b) Signed-off-by: Gengliang Wang <gengliang@apache.org>

…empt id not matching Remove the appAttemptId from TransportConf, and parsing through SparkEnv. Push based shuffle will fail if there are any attemptId set in the SparkConf, as the attemptId is not set correctly in Driver. No Tested within our Yarn cluster. Without this PR, the Driver will fail to finalize the shuffle merge on all the mergers. After the patch, Driver can successfully finalize the shuffle merge and the push based shuffle can work fine. Also with unit test to verify the attemptId is being set in the BlockStoreClient in Driver. Closes #34018 from zhouyejoe/SPARK-36772. Authored-by: Ye Zhou <yezhou@linkedin.com> Signed-off-by: Gengliang Wang <gengliang@apache.org>

[WIP][SPARK-36772] FinalizeShuffleMerge fails with an exception due t…

19dbefc

…o attempt id not matching.

github-actions bot added the CORE label Sep 16, 2021

Ngone51 reviewed Sep 16, 2021

View reviewed changes

core/src/main/scala/org/apache/spark/SparkEnv.scala Outdated Show resolved Hide resolved

mridulm reviewed Sep 16, 2021

View reviewed changes

common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/BlockStoreClient.java Outdated Show resolved Hide resolved

zhouyejoe added 2 commits September 16, 2021 12:08

Address comments. Make sure attemptId is set in Driver for merge fina…

a3f6cfb

…lization

Add unit test

9418ec8

Ngone51 reviewed Sep 17, 2021

View reviewed changes

core/src/main/scala/org/apache/spark/SparkContext.scala Outdated Show resolved Hide resolved

Ngone51 reviewed Sep 17, 2021

View reviewed changes

core/src/test/scala/org/apache/spark/SparkContextSuite.scala Outdated Show resolved Hide resolved

Ngone51 reviewed Sep 17, 2021

View reviewed changes

core/src/main/scala/org/apache/spark/SparkEnv.scala Outdated Show resolved Hide resolved

address comment.

1603998

zhouyejoe changed the title ~~[WIP][SPARK-36772] FinalizeShuffleMerge fails with an exception due to attempt id not matching~~ [SPARK-36772] FinalizeShuffleMerge fails with an exception due to attempt id not matching Sep 17, 2021

address comments

072361b

Ngone51 approved these changes Sep 17, 2021

View reviewed changes

mridulm reviewed Sep 17, 2021

View reviewed changes

...network-shuffle/src/main/java/org/apache/spark/network/shuffle/ExternalBlockStoreClient.java Outdated Show resolved Hide resolved

mridulm reviewed Sep 17, 2021

View reviewed changes

core/src/main/scala/org/apache/spark/SparkContext.scala Outdated Show resolved Hide resolved

mridulm reviewed Sep 17, 2021

View reviewed changes

core/src/test/scala/org/apache/spark/scheduler/CoarseGrainedSchedulerBackendSuite.scala Outdated Show resolved Hide resolved

venkata91 reviewed Sep 17, 2021

View reviewed changes

core/src/main/scala/org/apache/spark/executor/CoarseGrainedExecutorBackend.scala Outdated Show resolved Hide resolved

Address comments. Changed the type for appAttemptId in BlockStoreClie…

0062005

…nt from Int to String, but add comparableAppAttemptId in ExternalBlockStoreClient

mridulm reviewed Sep 18, 2021

View reviewed changes

...network-shuffle/src/main/java/org/apache/spark/network/shuffle/ExternalBlockStoreClient.java Outdated Show resolved Hide resolved

mridulm reviewed Sep 18, 2021

View reviewed changes

...network-shuffle/src/main/java/org/apache/spark/network/shuffle/ExternalBlockStoreClient.java Outdated Show resolved Hide resolved

address comments

f6e47b8

mridulm approved these changes Sep 18, 2021

View reviewed changes

gengliangwang approved these changes Sep 18, 2021

View reviewed changes

gengliangwang closed this in cabc36b Sep 18, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-36772] FinalizeShuffleMerge fails with an exception due to attempt id not matching #34018

[SPARK-36772] FinalizeShuffleMerge fails with an exception due to attempt id not matching #34018

zhouyejoe commented Sep 16, 2021 •

edited

zhouyejoe commented Sep 16, 2021

gengliangwang commented Sep 16, 2021 •

edited

zhouyejoe commented Sep 17, 2021

gengliangwang commented Sep 17, 2021

zhouyejoe commented Sep 17, 2021

mridulm commented Sep 17, 2021 •

edited

zhouyejoe commented Sep 17, 2021

Ngone51 commented Sep 17, 2021

zhouyejoe commented Sep 18, 2021

zhouyejoe commented Sep 18, 2021

mridulm left a comment

zhouyejoe commented Sep 18, 2021

mridulm commented Sep 18, 2021

SparkQA commented Sep 18, 2021

SparkQA commented Sep 18, 2021

gengliangwang commented Sep 18, 2021

SparkQA commented Sep 18, 2021

Ngone51 commented Sep 18, 2021

mridulm commented Sep 18, 2021

zhouyejoe commented Sep 19, 2021

[SPARK-36772] FinalizeShuffleMerge fails with an exception due to attempt id not matching #34018

[SPARK-36772] FinalizeShuffleMerge fails with an exception due to attempt id not matching #34018

Conversation

zhouyejoe commented Sep 16, 2021 • edited

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

zhouyejoe commented Sep 16, 2021

gengliangwang commented Sep 16, 2021 • edited

zhouyejoe commented Sep 17, 2021

gengliangwang commented Sep 17, 2021

zhouyejoe commented Sep 17, 2021

mridulm commented Sep 17, 2021 • edited

zhouyejoe commented Sep 17, 2021

Ngone51 commented Sep 17, 2021

zhouyejoe commented Sep 18, 2021

zhouyejoe commented Sep 18, 2021

mridulm left a comment

Choose a reason for hiding this comment

zhouyejoe commented Sep 18, 2021

mridulm commented Sep 18, 2021

SparkQA commented Sep 18, 2021

SparkQA commented Sep 18, 2021

gengliangwang commented Sep 18, 2021

SparkQA commented Sep 18, 2021

Ngone51 commented Sep 18, 2021

mridulm commented Sep 18, 2021

zhouyejoe commented Sep 19, 2021

zhouyejoe commented Sep 16, 2021 •

edited

gengliangwang commented Sep 16, 2021 •

edited

mridulm commented Sep 17, 2021 •

edited