[#1398] fix(mr,tez): Make attempId computable and move it to taskAttemptId in BlockId layout. #1418

qijiale76 · 2024-01-04T10:15:26Z

What changes were proposed in this pull request?

Before this PR, in MR and TEZ engine:

attemptId is in sequenceNo of BlockId instead of taskAttemptId.
attempId is fixed 6 bit.

After this PR:

attemptId is in taskAttemptId. This is more reasonable.
attempId is calculated from max num of allowed failures and whether speculative execution is enabled.

Why are the changes needed?

Fix: #1398

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Existing UT and integrated tests.

codecov-commenter · 2024-01-04T10:26:56Z

Codecov Report

Attention: Patch coverage is 79.10448% with 14 lines in your changes are missing coverage. Please review.

Project coverage is 54.88%. Comparing base (dd67774) to head (bc5585d).
Report is 4 commits behind head on master.

Files	Patch %	Lines
...c/main/java/org/apache/tez/common/RssTezUtils.java	76.19%	4 Missing and 1 partial ⚠️
...rg/apache/hadoop/mapred/RssMapOutputCollector.java	0.00%	3 Missing ⚠️
...library/common/shuffle/impl/RssTezFetcherTask.java	0.00%	3 Missing ⚠️
...library/common/shuffle/impl/RssShuffleManager.java	0.00%	2 Missing ⚠️
...n/java/org/apache/hadoop/mapreduce/RssMRUtils.java	95.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #1418      +/-   ##
============================================
+ Coverage     54.01%   54.88%   +0.87%     
- Complexity     2863     2868       +5     
============================================
  Files           438      418      -20     
  Lines         24850    22552    -2298     
  Branches       2114     2120       +6     
============================================
- Hits          13423    12378    -1045     
+ Misses        10586     9406    -1180     
+ Partials        841      768      -73

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

qijiale76 · 2024-01-04T10:48:54Z

@jerqi Could you please provide suggestions on areas that need improvement?

zuston · 2024-01-08T09:33:38Z

cc @zhengchenyu could you help review this ?

zhengchenyu · 2024-02-27T06:43:44Z

Sine #1529 is merged into master, I think we should review this PR?
After this PR, the blockid calculation for spark, mr, tez will remain consistent. Then we will reduce the probability of overflow problems.

@qijiale76 Can you reconstruct the code according to #1529?

zuston · 2024-02-29T03:48:30Z

@qijiale76 Do you want to push this forward?

qijiale76 · 2024-02-29T04:30:42Z

@qijiale76 Do you want to push this forward?

Yes, I’ll reconstruct the code next week.

… taskAttemptId in BlockId.

client-mr/core/src/main/java/org/apache/hadoop/mapreduce/RssMRUtils.java

client-tez/src/main/java/org/apache/tez/common/RssTezUtils.java

client-mr/core/src/main/java/org/apache/hadoop/mapreduce/RssMRUtils.java

client-tez/src/main/java/org/apache/tez/common/RssTezUtils.java

github-actions · 2024-03-21T13:37:00Z

Test Results

2 657 files +10 2 657 suites +10 5h 30m 55s ⏱️ + 4m 6s
946 tests ± 0 944 ✅ ± 0 1 💤 ±0 0 ❌ ±0 1 🔥 ±0
11 799 runs +20 11 783 ✅ +20 15 💤 ±0 0 ❌ ±0 1 🔥 ±0

For more details on these errors, see this check.

Results for commit a543fd2. ± Comparison against base commit e0a2e3d.

This pull request removes 2 and adds 2 tests. Note that renamed tests count towards both.

org.apache.uniffle.shuffle.manager.RssShuffleManagerBaseTest ‑ testGetAttemptIdBits
org.apache.uniffle.shuffle.manager.RssShuffleManagerBaseTest ‑ testGetMaxAttemptNo

org.apache.uniffle.client.ClientUtilsTest ‑ testGetMaxAttemptNo
org.apache.uniffle.client.ClientUtilsTest ‑ testGetNumberOfSignificantBits

♻️ This comment has been updated with latest results.

…m long to int."

qijiale76 · 2024-03-28T11:44:43Z

@EnricoMi Thanks for your very helpful review. I have updated this PR based on your suggestions and by referring to Spark's implementation. Could you please review the latest code again?

client/src/main/java/org/apache/uniffle/client/util/ClientUtils.java

client-mr/core/src/main/java/org/apache/hadoop/mapreduce/RssMRUtils.java

client-mr/core/src/main/java/org/apache/hadoop/mapreduce/task/reduce/RssEventFetcher.java

client-tez/src/main/java/org/apache/tez/common/RssTezUtils.java

...t-tez/src/main/java/org/apache/tez/runtime/library/output/RssOrderedPartitionedKVOutput.java

...tez/src/main/java/org/apache/tez/runtime/library/output/RssUnorderedPartitionedKVOutput.java

common/src/main/java/org/apache/uniffle/common/util/BlockIdLayout.java

client-mr/core/src/main/java/org/apache/hadoop/mapreduce/RssMRUtils.java

client-tez/src/main/java/org/apache/tez/common/RssTezUtils.java

common/src/main/java/org/apache/uniffle/common/util/BlockIdLayout.java

client-mr/core/src/main/java/org/apache/hadoop/mapred/SortWriteBufferManager.java

# Conflicts: # client-spark/common/src/main/java/org/apache/uniffle/shuffle/manager/RssShuffleManagerBase.java # client-tez/src/main/java/org/apache/tez/runtime/library/common/shuffle/impl/RssTezFetcherTask.java

qijiale76 · 2024-06-28T06:23:18Z

@EnricoMi, thank you very much for your reviews. I have reverted taskAttemptIds to long in the MR, Tez, and Spark engine clients. Could you please review this PR again?

zhengchenyu · 2024-07-04T02:33:01Z

@jerqi I still think taskattemptid should be int. Since we've limited taskattemptid to no more than 32 bits, int is enough. For spark, taskattemptid has changed, now does not comes from TaskContext::taskAttemptId.

See the code:

incubator-uniffle/common/src/main/java/org/apache/uniffle/common/util/BlockIdLayout.java

Line 69 in f7c6d2d

if (sequenceNoBits >= 32 || partitionIdBits >= 32 || taskAttemptIdBits >= 32) {

EnricoMi · 2024-07-04T16:00:53Z

Code paths that work fine with long taskAttemptIds should show that via using long, paths that are limited to int ids should indicate that using int. Then you know what the code supports and which code has limitations.

EnricoMi

LGTM!

zuston requested a review from zhengchenyu January 8, 2024 09:33

fix(MR)(TEZ): Limit attemptId to 4 bit and move it from sequenceNo to…

f2bbf08

… taskAttemptId in BlockId.

qijiale76 force-pushed the issue#1398 branch from 239c418 to f2bbf08 Compare March 21, 2024 07:34

EnricoMi reviewed Mar 21, 2024

View reviewed changes

qijiale76 added 5 commits March 22, 2024 19:57

According to the review, modify the code and change taskAttemptId fro…

ba15386

…m long to int."

Resolve failed tests.

8787f0d

Resolve failed tests.

8fdfda5

Resolve tez bug.

08311bc

Calculate attemptBits from conf.

fdca0a5

jerqi changed the title ~~[#1398] fix(MR)(TEZ): Limit attemptId to 4 bit and move it from 18 bit atomicInt to 21 bit taskAttemptId in 63 bit BlockId.~~ [#1398] fix(mr,tez): Limit attemptId to 4 bit and move it from 18 bit atomicInt to 21 bit taskAttemptId in 63 bit BlockId. Mar 28, 2024

Resolve failed checkstyle.

f84ad00

qijiale76 requested a review from EnricoMi March 28, 2024 11:19

qijiale76 changed the title ~~[#1398] fix(mr,tez): Limit attemptId to 4 bit and move it from 18 bit atomicInt to 21 bit taskAttemptId in 63 bit BlockId.~~ [#1398] fix(mr,tez): Make attempId computable and move it to taskAttemptId in BlockId layout. Mar 28, 2024

qijiale76 marked this pull request as ready for review March 28, 2024 11:40

EnricoMi reviewed Mar 28, 2024

View reviewed changes

qijiale76 added 2 commits March 29, 2024 15:53

Update the code according to the review.

ee21967

fix checkstyle.

bc5585d

qijiale76 requested a review from EnricoMi April 17, 2024 10:39

EnricoMi requested changes Apr 18, 2024

View reviewed changes

qijiale76 mentioned this pull request Apr 23, 2024

[#1341] fix(mr): Fix MR Combiner ArrayIndexOutOfBoundsException Bug. #1666

Merged

qijiale76 added 2 commits June 27, 2024 19:25

Merge branch 'master_github' into issue#1398

0e780fe

# Conflicts: # client-spark/common/src/main/java/org/apache/uniffle/shuffle/manager/RssShuffleManagerBase.java # client-tez/src/main/java/org/apache/tez/runtime/library/common/shuffle/impl/RssTezFetcherTask.java

revert taskAttemptId to long

d4d8e15

qijiale76 requested a review from EnricoMi June 28, 2024 06:18

qijiale76 requested a review from jerqi July 2, 2024 02:24

qijiale76 added 2 commits July 3, 2024 18:36

revert all taskAttemptId to long

facec3c

revert unnecessary code modification

a543fd2

EnricoMi approved these changes Jul 4, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#1398] fix(mr,tez): Make attempId computable and move it to taskAttemptId in BlockId layout. #1418

[#1398] fix(mr,tez): Make attempId computable and move it to taskAttemptId in BlockId layout. #1418

qijiale76 commented Jan 4, 2024 •

edited

Loading

codecov-commenter commented Jan 4, 2024 •

edited

Loading

qijiale76 commented Jan 4, 2024

zuston commented Jan 8, 2024

zhengchenyu commented Feb 27, 2024

zuston commented Feb 29, 2024

qijiale76 commented Feb 29, 2024

github-actions bot commented Mar 21, 2024 •

edited

Loading

qijiale76 commented Mar 28, 2024

qijiale76 commented Jun 28, 2024

zhengchenyu commented Jul 4, 2024

EnricoMi commented Jul 4, 2024 •

edited by rickyma

Loading

EnricoMi left a comment

[#1398] fix(mr,tez): Make attempId computable and move it to taskAttemptId in BlockId layout. #1418

Are you sure you want to change the base?

[#1398] fix(mr,tez): Make attempId computable and move it to taskAttemptId in BlockId layout. #1418

Conversation

qijiale76 commented Jan 4, 2024 • edited Loading

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?

codecov-commenter commented Jan 4, 2024 • edited Loading

Codecov Report

qijiale76 commented Jan 4, 2024

zuston commented Jan 8, 2024

zhengchenyu commented Feb 27, 2024

zuston commented Feb 29, 2024

qijiale76 commented Feb 29, 2024

github-actions bot commented Mar 21, 2024 • edited Loading

Test Results

qijiale76 commented Mar 28, 2024

qijiale76 commented Jun 28, 2024

zhengchenyu commented Jul 4, 2024

EnricoMi commented Jul 4, 2024 • edited by rickyma Loading

EnricoMi left a comment

Choose a reason for hiding this comment

qijiale76 commented Jan 4, 2024 •

edited

Loading

codecov-commenter commented Jan 4, 2024 •

edited

Loading

github-actions bot commented Mar 21, 2024 •

edited

Loading

EnricoMi commented Jul 4, 2024 •

edited by rickyma

Loading