You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@uniffle.apache.org by GitBox <gi...@apache.org> on 2022/07/26 07:27:15 UTC
[GitHub] [incubator-uniffle] xianjingfeng opened a new pull request, #72: [Improvement]LocalStorage init use multi thread #71
xianjingfeng opened a new pull request, #72:
URL: https://github.com/apache/incubator-uniffle/pull/72
issue: #71
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] colinmjj commented on pull request #72: [Improvement]LocalStorage init use multi thread #71
Posted by GitBox <gi...@apache.org>.
colinmjj commented on PR #72:
URL: https://github.com/apache/incubator-uniffle/pull/72#issuecomment-1195154202
Test case is needed for this improvement
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] jerqi merged pull request #72: [Improvement]LocalStorage init use multi thread #71
Posted by GitBox <gi...@apache.org>.
jerqi merged PR #72:
URL: https://github.com/apache/incubator-uniffle/pull/72
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] jerqi commented on pull request #72: [Improvement]LocalStorage init use multi thread #71
Posted by GitBox <gi...@apache.org>.
jerqi commented on PR #72:
URL: https://github.com/apache/incubator-uniffle/pull/72#issuecomment-1195170392
Could you modify the description according to our pull request template? We will use the description as commit message, the commit message will provide more information to help people understand the code.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] jerqi commented on a diff in pull request #72: [Improvement]LocalStorage init use multi thread #71
Posted by GitBox <gi...@apache.org>.
jerqi commented on code in PR #72:
URL: https://github.com/apache/incubator-uniffle/pull/72#discussion_r932932987
##########
server/src/main/java/org/apache/uniffle/server/storage/LocalStorageManager.java:
##########
@@ -63,15 +71,45 @@ public class LocalStorageManager extends SingleStorageManager {
if (highWaterMarkOfWrite < lowWaterMarkOfWrite) {
throw new IllegalArgumentException("highWaterMarkOfWrite must be larger than lowWaterMarkOfWrite");
}
- for (String storagePath : storageBasePaths) {
- localStorages.add(LocalStorage.newBuilder()
- .basePath(storagePath)
- .capacity(capacity)
- .lowWaterMarkOfWrite(lowWaterMarkOfWrite)
- .highWaterMarkOfWrite(highWaterMarkOfWrite)
- .shuffleExpiredTimeoutMs(shuffleExpiredTimeoutMs)
- .build());
+
+ // We must make sure the order of `storageBasePaths` and `localStorages` is same, or some unit test may be fail
+ CountDownLatch countDownLatch = new CountDownLatch(storageBasePaths.length);
+ AtomicInteger successCount = new AtomicInteger();
+ ExecutorService executorService = Executors.newCachedThreadPool();
+ LocalStorage[] localStorageArray = new LocalStorage[storageBasePaths.length];
+ for (int i = 0; i < storageBasePaths.length; i++) {
+ final int idx = i;
+ String storagePath = storageBasePaths[i];
+ executorService.submit(() -> {
+ try {
+ localStorageArray[idx] = LocalStorage.newBuilder()
+ .basePath(storagePath)
+ .capacity(capacity)
+ .lowWaterMarkOfWrite(lowWaterMarkOfWrite)
+ .highWaterMarkOfWrite(highWaterMarkOfWrite)
+ .shuffleExpiredTimeoutMs(shuffleExpiredTimeoutMs)
+ .build();
+ successCount.incrementAndGet();
+ } catch (Exception e) {
+ LOG.error("LocalStorage init failed!", e);
+ } finally {
+ countDownLatch.countDown();
+ }
+ });
+ }
+
+ try {
+ countDownLatch.await();
+ } catch (InterruptedException e) {
+ e.printStackTrace();
}
+
+ int failedCount = storageBasePaths.length - successCount.get();
+ if (failedCount > 0) {
+ throw new RuntimeException(String.format("[%s] local storage init failed!", failedCount));
+ }
+
Review Comment:
Should we shutdown the executor pool after we finished to use them?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] colinmjj commented on a diff in pull request #72: [Improvement]LocalStorage init use multi thread #71
Posted by GitBox <gi...@apache.org>.
colinmjj commented on code in PR #72:
URL: https://github.com/apache/incubator-uniffle/pull/72#discussion_r929659644
##########
server/src/main/java/org/apache/uniffle/server/storage/LocalStorageManager.java:
##########
@@ -63,18 +65,43 @@ public class LocalStorageManager extends SingleStorageManager {
if (highWaterMarkOfWrite < lowWaterMarkOfWrite) {
throw new IllegalArgumentException("highWaterMarkOfWrite must be larger than lowWaterMarkOfWrite");
}
+
+ CountDownLatch countDownLatch = new CountDownLatch(storageBasePaths.length);
+ AtomicInteger successCount = new AtomicInteger();
for (String storagePath : storageBasePaths) {
- localStorages.add(LocalStorage.newBuilder()
- .basePath(storagePath)
- .capacity(capacity)
- .lowWaterMarkOfWrite(lowWaterMarkOfWrite)
- .highWaterMarkOfWrite(highWaterMarkOfWrite)
- .shuffleExpiredTimeoutMs(shuffleExpiredTimeoutMs)
- .build());
+ new Thread(() -> {
+ try {
+ addLocalStorage(LocalStorage.newBuilder()
+ .basePath(storagePath)
+ .capacity(capacity)
+ .lowWaterMarkOfWrite(lowWaterMarkOfWrite)
+ .highWaterMarkOfWrite(highWaterMarkOfWrite)
+ .shuffleExpiredTimeoutMs(shuffleExpiredTimeoutMs)
+ .build());
+ successCount.incrementAndGet();
+ } finally {
+ countDownLatch.countDown();
+ }
+ }).start();
+ }
+ try {
+ countDownLatch.await();
+ } catch (InterruptedException e) {
+ e.printStackTrace();
+ }
+
+ int failedCount = storageBasePaths.length - successCount.get();
+ if (failedCount > 0) {
+ throw new RuntimeException(String.format("[%s] local storage init failed!", failedCount));
}
+
this.checker = new LocalStorageChecker(conf, localStorages);
}
+ private synchronized void addLocalStorage(LocalStorage localStorage) {
Review Comment:
Does it do the clean concurrently with synchronized?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] codecov-commenter commented on pull request #72: [Improvement]LocalStorage init use multi thread #71
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #72:
URL: https://github.com/apache/incubator-uniffle/pull/72#issuecomment-1195173153
# [Codecov](https://codecov.io/gh/apache/incubator-uniffle/pull/72?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#72](https://codecov.io/gh/apache/incubator-uniffle/pull/72?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (c2b8a6d) into [master](https://codecov.io/gh/apache/incubator-uniffle/commit/aa18be05ab7c00cd04b98134c11fdbe898893e95?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (aa18be0) will **increase** coverage by `0.57%`.
> The diff coverage is `81.81%`.
```diff
@@ Coverage Diff @@
## master #72 +/- ##
============================================
+ Coverage 56.39% 56.96% +0.57%
+ Complexity 1173 1052 -121
============================================
Files 149 136 -13
Lines 7953 6755 -1198
Branches 761 649 -112
============================================
- Hits 4485 3848 -637
+ Misses 3226 2692 -534
+ Partials 242 215 -27
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-uniffle/pull/72?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...he/uniffle/server/storage/LocalStorageManager.java](https://codecov.io/gh/apache/incubator-uniffle/pull/72/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2VydmVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS91bmlmZmxlL3NlcnZlci9zdG9yYWdlL0xvY2FsU3RvcmFnZU1hbmFnZXIuamF2YQ==) | `54.25% <81.81%> (+3.62%)` | :arrow_up: |
| [...storage/handler/impl/DataSkippableReadHandler.java](https://codecov.io/gh/apache/incubator-uniffle/pull/72/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3RvcmFnZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvdW5pZmZsZS9zdG9yYWdlL2hhbmRsZXIvaW1wbC9EYXRhU2tpcHBhYmxlUmVhZEhhbmRsZXIuamF2YQ==) | `81.25% <0.00%> (-3.13%)` | :arrow_down: |
| [...org/apache/uniffle/server/ShuffleFlushManager.java](https://codecov.io/gh/apache/incubator-uniffle/pull/72/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2VydmVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS91bmlmZmxlL3NlcnZlci9TaHVmZmxlRmx1c2hNYW5hZ2VyLmphdmE=) | `76.70% <0.00%> (-1.71%)` | :arrow_down: |
| [...e/uniffle/server/storage/SingleStorageManager.java](https://codecov.io/gh/apache/incubator-uniffle/pull/72/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2VydmVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS91bmlmZmxlL3NlcnZlci9zdG9yYWdlL1NpbmdsZVN0b3JhZ2VNYW5hZ2VyLmphdmE=) | `65.57% <0.00%> (-1.64%)` | :arrow_down: |
| [.../apache/uniffle/coordinator/ClientConfManager.java](https://codecov.io/gh/apache/incubator-uniffle/pull/72/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29vcmRpbmF0b3Ivc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3VuaWZmbGUvY29vcmRpbmF0b3IvQ2xpZW50Q29uZk1hbmFnZXIuamF2YQ==) | `91.54% <0.00%> (-1.41%)` | :arrow_down: |
| [.../java/org/apache/hadoop/mapreduce/RssMRConfig.java](https://codecov.io/gh/apache/incubator-uniffle/pull/72/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y2xpZW50LW1yL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9oYWRvb3AvbWFwcmVkdWNlL1Jzc01SQ29uZmlnLmphdmE=) | | |
| [...java/org/apache/hadoop/mapred/SortWriteBuffer.java](https://codecov.io/gh/apache/incubator-uniffle/pull/72/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y2xpZW50LW1yL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9oYWRvb3AvbWFwcmVkL1NvcnRXcml0ZUJ1ZmZlci5qYXZh) | | |
| [...n/java/org/apache/hadoop/mapreduce/MRIdHelper.java](https://codecov.io/gh/apache/incubator-uniffle/pull/72/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y2xpZW50LW1yL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9oYWRvb3AvbWFwcmVkdWNlL01SSWRIZWxwZXIuamF2YQ==) | | |
| [...mapreduce/task/reduce/RssInMemoryRemoteMerger.java](https://codecov.io/gh/apache/incubator-uniffle/pull/72/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y2xpZW50LW1yL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9oYWRvb3AvbWFwcmVkdWNlL3Rhc2svcmVkdWNlL1Jzc0luTWVtb3J5UmVtb3RlTWVyZ2VyLmphdmE=) | | |
| [...pache/hadoop/mapreduce/task/reduce/RssShuffle.java](https://codecov.io/gh/apache/incubator-uniffle/pull/72/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y2xpZW50LW1yL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9oYWRvb3AvbWFwcmVkdWNlL3Rhc2svcmVkdWNlL1Jzc1NodWZmbGUuamF2YQ==) | | |
| ... and [8 more](https://codecov.io/gh/apache/incubator-uniffle/pull/72/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] xianjingfeng commented on a diff in pull request #72: [Improvement]LocalStorage init use multi thread #71
Posted by GitBox <gi...@apache.org>.
xianjingfeng commented on code in PR #72:
URL: https://github.com/apache/incubator-uniffle/pull/72#discussion_r929677130
##########
server/src/main/java/org/apache/uniffle/server/storage/LocalStorageManager.java:
##########
@@ -63,18 +65,43 @@ public class LocalStorageManager extends SingleStorageManager {
if (highWaterMarkOfWrite < lowWaterMarkOfWrite) {
throw new IllegalArgumentException("highWaterMarkOfWrite must be larger than lowWaterMarkOfWrite");
}
+
+ CountDownLatch countDownLatch = new CountDownLatch(storageBasePaths.length);
+ AtomicInteger successCount = new AtomicInteger();
for (String storagePath : storageBasePaths) {
- localStorages.add(LocalStorage.newBuilder()
- .basePath(storagePath)
- .capacity(capacity)
- .lowWaterMarkOfWrite(lowWaterMarkOfWrite)
- .highWaterMarkOfWrite(highWaterMarkOfWrite)
- .shuffleExpiredTimeoutMs(shuffleExpiredTimeoutMs)
- .build());
+ new Thread(() -> {
+ try {
+ addLocalStorage(LocalStorage.newBuilder()
+ .basePath(storagePath)
+ .capacity(capacity)
+ .lowWaterMarkOfWrite(lowWaterMarkOfWrite)
+ .highWaterMarkOfWrite(highWaterMarkOfWrite)
+ .shuffleExpiredTimeoutMs(shuffleExpiredTimeoutMs)
+ .build());
+ successCount.incrementAndGet();
+ } finally {
+ countDownLatch.countDown();
+ }
+ }).start();
+ }
+ try {
+ countDownLatch.await();
+ } catch (InterruptedException e) {
+ e.printStackTrace();
+ }
+
+ int failedCount = storageBasePaths.length - successCount.get();
+ if (failedCount > 0) {
+ throw new RuntimeException(String.format("[%s] local storage init failed!", failedCount));
}
+
this.checker = new LocalStorageChecker(conf, localStorages);
}
+ private synchronized void addLocalStorage(LocalStorage localStorage) {
Review Comment:
no, clean do before `addLocalStorage`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] frankliee commented on a diff in pull request #72: [Improvement]LocalStorage init use multi thread #71
Posted by GitBox <gi...@apache.org>.
frankliee commented on code in PR #72:
URL: https://github.com/apache/incubator-uniffle/pull/72#discussion_r929681750
##########
server/src/main/java/org/apache/uniffle/server/storage/LocalStorageManager.java:
##########
@@ -63,18 +65,43 @@ public class LocalStorageManager extends SingleStorageManager {
if (highWaterMarkOfWrite < lowWaterMarkOfWrite) {
throw new IllegalArgumentException("highWaterMarkOfWrite must be larger than lowWaterMarkOfWrite");
}
+
+ CountDownLatch countDownLatch = new CountDownLatch(storageBasePaths.length);
+ AtomicInteger successCount = new AtomicInteger();
for (String storagePath : storageBasePaths) {
- localStorages.add(LocalStorage.newBuilder()
- .basePath(storagePath)
- .capacity(capacity)
- .lowWaterMarkOfWrite(lowWaterMarkOfWrite)
- .highWaterMarkOfWrite(highWaterMarkOfWrite)
- .shuffleExpiredTimeoutMs(shuffleExpiredTimeoutMs)
- .build());
+ new Thread(() -> {
+ try {
Review Comment:
I prefer to leverage more graceful `parallelStream`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org