Skip to content

[Refactor] Optimize creating shuffle handlers #259

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.

Sign up for GitHub

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

Jump to bottom
Merged
jerqi merged 3 commits into from
Oct 12, 2022
Merged

[Refactor] Optimize creating shuffle handlers #259

jerqi merged 3 commits into from
Oct 12, 2022

Conversation

@zuston
Copy link
Member

@zuston zuston commented Oct 10, 2022

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 jerqi October 10, 2022 09:50
@zuston
Copy link
Member Author

zuston commented Oct 10, 2022

PTAL @jerqi @frankliee

@codecov-commenter
Copy link

codecov-commenter commented Oct 10, 2022 •
edited
Loading

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

@jerqi
Copy link
Contributor

jerqi commented Oct 10, 2022

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
storage/src/main/java/org/apache/uniffle/storage/util/StorageType.java
LOCALFILE_HDFS(6),
MEMORY_LOCALFILE(3),
MEMORY_HDFS(5),
MEMORY_LOCALFILE_HDFS(7);
Copy link
Contributor

@frankliee frankliee Oct 11, 2022

Choose a reason for hiding this comment

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

Why this order？

Copy link
Member Author

@zuston zuston Oct 11, 2022

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
Copy link
Member Author

zuston commented Oct 11, 2022

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
@jerqi
Copy link
Contributor

jerqi commented Oct 11, 2022

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
Copy link
Member Author

zuston commented Oct 11, 2022

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.

jerqi
jerqi approved these changes Oct 11, 2022
View reviewed changes
Copy link
Contributor

@jerqi jerqi left a comment

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
Copy link
Member Author

zuston commented Oct 11, 2022

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

@zuston
Copy link
Member Author

zuston commented Oct 12, 2022

Do u mind merging this ? @jerqi

@jerqi jerqi merged commit 4d2db5b into apache:master Oct 12, 2022
@jerqi
Copy link
Contributor

jerqi commented Oct 12, 2022

Do u mind merging this ? @jerqi

merged.

@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

@jerqi jerqi jerqi 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 @jerqi @frankliee