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

[improvement] Remove experimental feature with ShuffleUploader #51

Merged
merged 2 commits into from Jul 12, 2022

Conversation

colinmjj
Copy link

What changes were proposed in this pull request?

ShuffleUploader is an experimental feature which is target to merge and upload data from local disk to remote storage if local disk hasn't enough space. It is replaced by MEMORY_LOCAL_HDFS and should be removed to avoid maintenance in the future.

Why are the changes needed?

The experimental feature is redundancy and we have to maintain it during update with new features about storage.

Does this PR introduce any user-facing change?

No

How was this patch tested?

with exist UTs

@colinmjj colinmjj requested a review from duanmeng July 12, 2022 03:01
@codecov-commenter
Copy link

codecov-commenter commented Jul 12, 2022

Codecov Report

Merging #51 (1ba35ba) into master (cbe39c1) will decrease coverage by 1.48%.
The diff coverage is 66.66%.

@@             Coverage Diff              @@
##             master      #51      +/-   ##
============================================
- Coverage     56.86%   55.38%   -1.49%     
+ Complexity     1208     1107     -101     
============================================
  Files           152      145       -7     
  Lines          8437     7778     -659     
  Branches        819      749      -70     
============================================
- Hits           4798     4308     -490     
+ Misses         3378     3230     -148     
+ Partials        261      240      -21     
Impacted Files Coverage Δ
...va/org/apache/uniffle/client/util/ClientUtils.java 18.18% <0.00%> (+0.79%) ⬆️
...a/org/apache/uniffle/server/ShuffleServerConf.java 99.13% <ø> (-0.18%) ⬇️
...uniffle/storage/factory/ShuffleHandlerFactory.java 0.00% <ø> (ø)
...ache/uniffle/storage/util/ShuffleStorageUtils.java 61.00% <ø> (-6.33%) ⬇️
...a/org/apache/uniffle/storage/util/StorageType.java 0.00% <ø> (ø)
...he/uniffle/server/storage/MultiStorageManager.java 37.50% <100.00%> (+9.63%) ⬆️
.../uniffle/server/storage/StorageManagerFactory.java 58.33% <100.00%> (+4.48%) ⬆️
...iffle/storage/handler/impl/ShuffleIndexHeader.java 0.00% <0.00%> (-77.56%) ⬇️
...e/uniffle/storage/handler/impl/HdfsFileWriter.java 55.22% <0.00%> (-28.36%) ⬇️
...storage/handler/impl/DataSkippableReadHandler.java 84.37% <0.00%> (-9.38%) ⬇️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cbe39c1...1ba35ba. Read the comment docs.

@colinmjj colinmjj requested a review from jerqi July 12, 2022 06:08
@jerqi
Copy link
Contributor

jerqi commented Jul 12, 2022

MultiStorageFaultToleranceTest have a useful case hdfsFallbackTest. We shouldn't remove it. It verify that we can write data to local disk, when we fail to write data on hdfs.

@colinmjj
Copy link
Author

MultiStorageFaultToleranceTest have a useful case hdfsFallbackTest. We shouldn't remove it. It verify that we can write data to local disk, when we fail to write data on hdfs.

ok, it should be recovered

@colinmjj colinmjj changed the title Remove experimental feature with ShuffleUploader [improvement] Remove experimental feature with ShuffleUploader Jul 12, 2022
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

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