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

[AQE][LocalOrder] Fix wrong param of expectedTaskIds in LocalOrderSegmentSplit #319

Merged
merged 1 commit into from
Nov 14, 2022

Conversation

zuston
Copy link
Member

@zuston zuston commented Nov 13, 2022

What changes were proposed in this pull request?

  1. Fix wrong param of expectedTaskIds in LocalOrderSegmentSplitter
  2. Fix the LOCAL_ORDER type invalid when reading

Why are the changes needed?

In current codebase, the reads of LOCAL_ORDER is invalid. This PR is to fix it.

Does this PR introduce any user-facing change?

No

How was this patch tested?

Existing UTs of AQESkewedJoinWithLocalOrderTest cover this case.

@zuston
Copy link
Member Author

zuston commented Nov 13, 2022

Important fix of LOCAL_ORDER @jerqi

@codecov-commenter
Copy link

codecov-commenter commented Nov 13, 2022

Codecov Report

Merging #319 (117cc82) into master (4a3d2be) will increase coverage by 0.07%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##             master     #319      +/-   ##
============================================
+ Coverage     61.01%   61.09%   +0.07%     
- Complexity     1489     1494       +5     
============================================
  Files           185      185              
  Lines          9314     9325      +11     
  Branches        900      903       +3     
============================================
+ Hits           5683     5697      +14     
+ Misses         3326     3323       -3     
  Partials        305      305              
Impacted Files Coverage Δ
...handler/impl/LocalFileQuorumClientReadHandler.java 0.00% <ø> (ø)
...che/uniffle/client/impl/ShuffleReadClientImpl.java 90.09% <100.00%> (+2.72%) ⬆️
...ffle/common/segment/LocalOrderSegmentSplitter.java 90.62% <0.00%> (+2.38%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@zuston
Copy link
Member Author

zuston commented Nov 13, 2022

Error: Failed to execute goal on project uniffle-parent: Could not resolve dependencies for project org.apache.uniffle:uniffle-parent:pom:0.7.0-snapshot: Failed to collect dependencies at org.slf4j:slf4j-log4j12:jar:1.7.25: Failed to read artifact descriptor for org.slf4j:slf4j-log4j12:jar:1.7.25: Could not transfer artifact org.slf4j:slf4j-log4j12:pom:1.7.25 from/to central (https://repo1.maven.org/maven2): transfer failed for https://repo1.maven.org/maven2/org/slf4j/slf4j-log4j12/1.7.25/slf4j-log4j12-1.7.25.pom: Connect to repo1.maven.org:443 [repo1.maven.org/146.75.28.209] failed: Connection timed out (Connection timed out) -> [Help 1]

@jerqi
Copy link
Contributor

jerqi commented Nov 13, 2022

We should add a ut to cover this case, because our exist tests can't find this problem.

@zuston
Copy link
Member Author

zuston commented Nov 14, 2022

Emm... Actually AQESkewedJoinWithLocalOrderTest could be cover the bug of expectedTaskIds .
However because the distribution type of LOCAL_ORDER is invalid with the incorrect initialization of shuffleReadClientImpl, it fall back to the NORMAL segment splitter, which cause the AQESkewedJoinWithLocalOrderTest run successfully.

it's a coincidence

@jerqi
Copy link
Contributor

jerqi commented Nov 14, 2022

Emm... Actually AQESkewedJoinWithLocalOrderTest could be cover the bug of expectedTaskIds . However because the distribution type of LOCAL_ORDER is invalid with the incorrect initialization of shuffleReadClientImpl, it fall back to the NORMAL segment splitter, which cause the AQESkewedJoinWithLocalOrderTest run successfully.

it's a coincidence

OK. It don't influence our performance test result, do it?

@zuston
Copy link
Member Author

zuston commented Nov 14, 2022

Emm... Actually AQESkewedJoinWithLocalOrderTest could be cover the bug of expectedTaskIds . However because the distribution type of LOCAL_ORDER is invalid with the incorrect initialization of shuffleReadClientImpl, it fall back to the NORMAL segment splitter, which cause the AQESkewedJoinWithLocalOrderTest run successfully.
it's a coincidence

OK. It don't influence our performance test result, do it?

Emm... All bugs are caused by #137 subsequent refactors of changing the [startMapId, endMapid) to taskIds bitmap.

The performance results are based on the initial commit. So the result is OK.

@jerqi
Copy link
Contributor

jerqi commented Nov 14, 2022

Emm... Actually AQESkewedJoinWithLocalOrderTest could be cover the bug of expectedTaskIds . However because the distribution type of LOCAL_ORDER is invalid with the incorrect initialization of shuffleReadClientImpl, it fall back to the NORMAL segment splitter, which cause the AQESkewedJoinWithLocalOrderTest run successfully.
it's a coincidence

OK. It don't influence our performance test result, do it?

Emm... All bugs are caused by #137 subsequent refactors of changing the [startMapId, endMapid) to taskIds bitmap.

The performance results are based on the initial commit. So the result is OK.

OK. LGTM, thanks @zuston

@jerqi jerqi merged commit 7aa0117 into apache:master Nov 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants