-
Notifications
You must be signed in to change notification settings - Fork 141
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
[#678] improvement: Write hdfs files asynchronously when detectStorage
#680
Conversation
Codecov Report
@@ Coverage Diff @@
## master #680 +/- ##
============================================
+ Coverage 60.79% 63.04% +2.24%
- Complexity 1839 1840 +1
============================================
Files 221 207 -14
Lines 12655 10677 -1978
Branches 1069 1064 -5
============================================
- Hits 7694 6731 -963
+ Misses 4553 3600 -953
+ Partials 408 346 -62 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
We should avoid multple threads operate the same file. |
It is the same file, but it is written to a different path. |
Maybe we should use CountdownLaunch + (parallelStream or thread pool). |
This doesn't seem to be caused by this pr.
|
Same problem as #667 . |
...in/java/org/apache/uniffle/coordinator/strategy/storage/AppBalanceSelectStorageStrategy.java
Outdated
Show resolved
Hide resolved
...org/apache/uniffle/coordinator/strategy/storage/LowestIOSampleCostSelectStorageStrategy.java
Outdated
Show resolved
Hide resolved
...main/java/org/apache/uniffle/coordinator/strategy/storage/AbstractSelectStorageStrategy.java
Outdated
Show resolved
Hide resolved
...main/java/org/apache/uniffle/coordinator/strategy/storage/AbstractSelectStorageStrategy.java
Outdated
Show resolved
Hide resolved
...main/java/org/apache/uniffle/coordinator/strategy/storage/AbstractSelectStorageStrategy.java
Outdated
Show resolved
Hide resolved
...in/java/org/apache/uniffle/coordinator/strategy/storage/AppBalanceSelectStorageStrategy.java
Outdated
Show resolved
Hide resolved
...main/java/org/apache/uniffle/coordinator/strategy/storage/AbstractSelectStorageStrategy.java
Outdated
Show resolved
Hide resolved
...tor/src/main/java/org/apache/uniffle/coordinator/strategy/storage/SelectStorageStrategy.java
Outdated
Show resolved
Hide resolved
...main/java/org/apache/uniffle/coordinator/strategy/storage/AbstractSelectStorageStrategy.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally LGTM
@@ -90,6 +91,7 @@ public void selectStorageTest() throws Exception { | |||
assertEquals(0, applicationManager.getRemoteStoragePathRankValue().get(remoteStorage2).getAppNum().get()); | |||
|
|||
// compare with two remote path | |||
final Configuration configuration = new Configuration(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems this line is not needed anymore?
I didn't see the var configuration
is used in this method anywhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
* Different strategies will have different sorting methods of remote paths | ||
* @return A comparator is to calculate the RankValue | ||
*/ | ||
abstract Comparator<Map.Entry<String, RankValue>> getComparator(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's make this method as
protected Comparator<Map.Entry<String, RankValue>> getComparator() {
return null;
}
? So AppBalanceSelectorStorageStrategy
doesn't need to override this.
Thanks all! @advancedxy @xianjingfeng @jerqi |
…tStorage` (apache#680) ### What changes were proposed in this pull request? Write files to hdfs asynchronously and reduce the time of detectStorage ### Why are the changes needed? Fix: apache#678 ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Origin uts.
…tStorage` (apache#680) ### What changes were proposed in this pull request? Write files to hdfs asynchronously and reduce the time of detectStorage ### Why are the changes needed? Fix: apache#678 ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Origin uts.
What changes were proposed in this pull request?
Write files to hdfs asynchronously and reduce the time of detectStorage
Why are the changes needed?
Fix: #678
Does this PR introduce any user-facing change?
No.
How was this patch tested?
Origin uts.