[Refactor] Optimize creating shuffle handlers by zuston · Pull Request #259 · apache/uniffle · GitHub
Skip to content

[Refactor] Optimize creating shuffle handlers#259

Merged
roryqi merged 3 commits into
apache:masterfrom
zuston:opCode
Oct 12, 2022
Merged

[Refactor] Optimize creating shuffle handlers#259
roryqi merged 3 commits into
apache:masterfrom
zuston:opCode

Conversation

@zuston

@zuston zuston commented Oct 10, 2022

Copy link
Copy Markdown
Member

What changes were proposed in this pull request?

[Refactor] Optimize creating shuffle handlers

Why are the changes needed?

When creating shuffle handler, the code is too duplicate and complex.

Does this PR introduce any user-facing change?

No

How was this patch tested?

UTs

@zuston zuston requested review from frankliee and roryqi October 10, 2022 09:50
@zuston

zuston commented Oct 10, 2022

Copy link
Copy Markdown
Member Author

@codecov-commenter

codecov-commenter commented Oct 10, 2022 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #259 (271fc33) into master (5875eb3) will increase coverage by 0.44%.
The diff coverage is 20.00%.

@@             Coverage Diff              @@
##             master     #259      +/-   ##
============================================
+ Coverage     59.18%   59.63%   +0.44%     
- Complexity     1343     1348       +5     
============================================
  Files           163      163              
  Lines          8837     8779      -58     
  Branches        835      835              
============================================
+ Hits           5230     5235       +5     
+ Misses         3339     3272      -67     
- Partials        268      272       +4
Impacted Files Coverage Δ
...rg/apache/hadoop/mapred/RssMapOutputCollector.java 0.00% <0.00%> (ø)
...uniffle/storage/factory/ShuffleHandlerFactory.java 0.00% <0.00%> (ø)
...a/org/apache/uniffle/storage/util/StorageType.java 100.00% <100.00%> (+100.00%) ⬆️
...e/coordinator/AppBalanceSelectStorageStrategy.java 72.00% <0.00%> (-4.00%) ⬇️
...nator/LowestIOSampleCostSelectStorageStrategy.java 68.83% <0.00%> (-2.60%) ⬇️
...rg/apache/uniffle/storage/common/LocalStorage.java 42.75% <0.00%> (-2.07%) ⬇️
...org/apache/uniffle/server/ShuffleFlushManager.java 76.66% <0.00%> (-1.67%) ⬇️

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

@roryqi

roryqi commented Oct 10, 2022

Copy link
Copy Markdown
Contributor

We have the method isMemoryShuffleEnabled in the MR client and Spark client, you can modify them, too.

frankliee
frankliee reviewed Oct 11, 2022
View reviewed changes
Comment thread storage/src/main/java/org/apache/uniffle/storage/util/StorageType.java
LOCALFILE_HDFS(6),
MEMORY_LOCALFILE(3),
MEMORY_HDFS(5),
MEMORY_LOCALFILE_HDFS(7);

@frankliee frankliee Oct 11, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why this order？

@zuston zuston Oct 11, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Just follow the original order

@zuston

zuston commented Oct 11, 2022

Copy link
Copy Markdown
Member Author

We have the method isMemoryShuffleEnabled in the MR client and Spark client, you can modify them, too.

Done

@zuston zuston requested a review from frankliee October 11, 2022 05:58
@roryqi

roryqi commented Oct 11, 2022

Copy link
Copy Markdown
Contributor

We have the method isMemoryShuffleEnabled in the MR client and Spark client, you can modify them, too.

Done

You only modify the spark2 client. How about spark3 and mr?

@zuston

zuston commented Oct 11, 2022

Copy link
Copy Markdown
Member Author

We have the method isMemoryShuffleEnabled in the MR client and Spark client, you can modify them, too.

Done

You only modify the spark2 client. How about spark3 and mr?

Updated. I forgot it.

roryqi
roryqi approved these changes Oct 11, 2022
View reviewed changes

@roryqi roryqi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM, thanks @zuston @frankliee , wait for CI

@zuston

zuston commented Oct 11, 2022

Copy link
Copy Markdown
Member Author

Flaky test: https://github.com/apache/incubator-uniffle/actions/runs/3225484333/jobs/5277849404

@zuston

zuston commented Oct 12, 2022

Copy link
Copy Markdown
Member Author

Do u mind merging this ? @jerqi

@roryqi roryqi merged commit 4d2db5b into apache:master Oct 12, 2022
@roryqi

roryqi commented Oct 12, 2022

Copy link
Copy Markdown
Contributor

@zuston zuston mentioned this pull request Jun 28, 2023
Merged
kaijchen pushed a commit that referenced this pull request Jul 18, 2023
@zuston
[MINOR] refactor: use general method to check the remote storage exis…
e5e1b30 
…tence (#980)

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

In #259 , we introduce some general method to check the storage types. So
this patch is to refactor the remaining part of this.

### Why are the changes needed?

Refactor to optimize the remote storage checking.

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

No.

### How was this patch tested?

Existing UTs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@roryqi roryqi roryqi approved these changes
@frankliee frankliee Awaiting requested review from frankliee

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@zuston @codecov-commenter @roryqi @frankliee