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 2023/01/11 10:40:14 UTC
[GitHub] [incubator-uniffle] zuston opened a new pull request, #471: [ISSUE-378][HugePartition][Part-2] Introduce memory usage limit
zuston opened a new pull request, #471:
URL: https://github.com/apache/incubator-uniffle/pull/471
### What changes were proposed in this pull request?
1. Introduce memory usage limit for huge partition
### Why are the changes needed?
1. To solve the problems mentioned by #378
### Does this PR introduce _any_ user-facing change?
Yes
### How was this patch tested?
1. UTs
--
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] zuston commented on pull request #471: [ISSUE-378][HugePartition][Part-2] Introduce memory usage limit and data flush
Posted by GitBox <gi...@apache.org>.
zuston commented on PR #471:
URL: https://github.com/apache/incubator-uniffle/pull/471#issuecomment-1380260848
> I'm ok to add rss.server.huge-partition.size.threshold setting, which is used to add memory limit. I just think maybe we should reuse rss.server.single.buffer.flush.threshold instead of another rss.server.huge-partition.memory.limit.ratio settings?
This suggestion has been accepted 😁. Please review the latest 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] zuston commented on pull request #471: [ISSUE-378][HugePartition][Part-2] Introduce memory usage limit and data flush
Posted by GitBox <gi...@apache.org>.
zuston commented on PR #471:
URL: https://github.com/apache/incubator-uniffle/pull/471#issuecomment-1379753276
It seems I got your point. The conf of `rss.server.single.buffer.flush.threshold` should be specified especially for huge partition whatever the single buffer flush is enabled or not. Right?
--
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] zuston commented on a diff in pull request #471: [ISSUE-378][HugePartition][Part-2] Introduce memory usage limit and data flush
Posted by GitBox <gi...@apache.org>.
zuston commented on code in PR #471:
URL: https://github.com/apache/incubator-uniffle/pull/471#discussion_r1070751412
##########
server/src/main/java/org/apache/uniffle/server/ShuffleServerGrpcService.java:
##########
@@ -353,7 +353,22 @@ public void finishShuffle(FinishShuffleRequest req,
@Override
public void requireBuffer(RequireBufferRequest request,
StreamObserver<RequireBufferResponse> responseObserver) {
- long requireBufferId = shuffleServer.getShuffleTaskManager().requireBuffer(request.getRequireSize());
+ String appId = request.getAppId();
+ long requireBufferId;
+ if (appId == null) {
Review Comment:
Nice catch. Let me use `StringUtils.isEmpty` to check this.
--
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] advancedxy commented on a diff in pull request #471: [ISSUE-378][HugePartition][Part-2] Introduce memory usage limit and data flush
Posted by GitBox <gi...@apache.org>.
advancedxy commented on code in PR #471:
URL: https://github.com/apache/incubator-uniffle/pull/471#discussion_r1070806030
##########
server/src/main/java/org/apache/uniffle/server/ShuffleServerConf.java:
##########
@@ -342,6 +342,20 @@ public class ShuffleServerConf extends RssBaseConf {
.noDefaultValue()
.withDescription("The env key to get json source of local storage media provider");
+ public static final ConfigOption<Long> HUGE_PARTITION_SIZE_THRESHOLD = ConfigOptions
+ .key("rss.server.huge-partition.size.threshold")
+ .longType()
+ .defaultValue(20 * 1024 * 1024 * 1024L)
Review Comment:
Is 20GB has some meaningful background? It would be great if you could add some suggestion values in the doc description.
Also could you please update the configuration doc?
##########
server/src/main/java/org/apache/uniffle/server/ShuffleTaskManager.java:
##########
@@ -200,7 +199,13 @@ public StatusCode registerShuffle(
public StatusCode cacheShuffleData(
String appId, int shuffleId, boolean isPreAllocated, ShufflePartitionedData spd) {
refreshAppId(appId);
- return shuffleBufferManager.cacheShuffleData(appId, shuffleId, isPreAllocated, spd);
+ return shuffleBufferManager.cacheShuffleData(
+ appId,
+ shuffleId,
+ isPreAllocated,
+ spd,
+ this::getPartitionDataSize
Review Comment:
Why couldn't we pass the partitionDataSize directly?
--
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] zuston commented on a diff in pull request #471: [ISSUE-378][HugePartition][Part-2] Introduce memory usage limit and data flush
Posted by GitBox <gi...@apache.org>.
zuston commented on code in PR #471:
URL: https://github.com/apache/incubator-uniffle/pull/471#discussion_r1070766298
##########
server/src/main/java/org/apache/uniffle/server/ShuffleServerGrpcService.java:
##########
@@ -353,7 +353,22 @@ public void finishShuffle(FinishShuffleRequest req,
@Override
public void requireBuffer(RequireBufferRequest request,
StreamObserver<RequireBufferResponse> responseObserver) {
- long requireBufferId = shuffleServer.getShuffleTaskManager().requireBuffer(request.getRequireSize());
+ String appId = request.getAppId();
+ long requireBufferId;
+ if (appId == null) {
Review Comment:
Done
##########
server/src/main/java/org/apache/uniffle/server/ShuffleTaskManager.java:
##########
@@ -200,7 +199,13 @@ public StatusCode registerShuffle(
public StatusCode cacheShuffleData(
String appId, int shuffleId, boolean isPreAllocated, ShufflePartitionedData spd) {
refreshAppId(appId);
- return shuffleBufferManager.cacheShuffleData(appId, shuffleId, isPreAllocated, spd);
+ return shuffleBufferManager.cacheShuffleData(
+ appId,
+ shuffleId,
+ isPreAllocated,
+ spd,
+ (appIdx, shuffleIdx, partitionIdx) -> getPartitionDataSize(appIdx, shuffleIdx, partitionIdx)
Review Comment:
> Java prefers to use Method Reference directly? -> this::getPartitionDataSize
Got it. Done
> I have another concern about this, getPartitionDataSize refers shuffleTaskInfos, which might leaking shuffleTaskInfos to ShuffleBufferManager, is there any better ways?
Emm... I have no ideas
--
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] zuston commented on pull request #471: [ISSUE-378][HugePartition][Part-2] Introduce memory usage limit and data flush
Posted by GitBox <gi...@apache.org>.
zuston commented on PR #471:
URL: https://github.com/apache/incubator-uniffle/pull/471#issuecomment-1384744342
Merged. Thanks for your review @advancedxy
--
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 #471: [ISSUE-378][HugePartition][Part-2] Introduce memory usage limit and data flush
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #471:
URL: https://github.com/apache/incubator-uniffle/pull/471#issuecomment-1379882130
# [Codecov](https://codecov.io/gh/apache/incubator-uniffle/pull/471?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 [#471](https://codecov.io/gh/apache/incubator-uniffle/pull/471?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (9907b6f) into [master](https://codecov.io/gh/apache/incubator-uniffle/commit/19a8bac456feb3b5be2000027e4719396300b4a3?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (19a8bac) will **decrease** coverage by `0.57%`.
> The diff coverage is `70.37%`.
```diff
@@ Coverage Diff @@
## master #471 +/- ##
============================================
- Coverage 58.78% 58.20% -0.58%
+ Complexity 1704 1494 -210
============================================
Files 206 184 -22
Lines 11471 9684 -1787
Branches 1024 873 -151
============================================
- Hits 6743 5637 -1106
+ Misses 4317 3676 -641
+ Partials 411 371 -40
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-uniffle/pull/471?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...pache/uniffle/server/ShuffleServerGrpcService.java](https://codecov.io/gh/apache/incubator-uniffle/pull/471?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2VydmVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS91bmlmZmxlL3NlcnZlci9TaHVmZmxlU2VydmVyR3JwY1NlcnZpY2UuamF2YQ==) | `0.79% <0.00%> (-0.02%)` | :arrow_down: |
| [...he/uniffle/server/buffer/ShuffleBufferManager.java](https://codecov.io/gh/apache/incubator-uniffle/pull/471?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2VydmVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS91bmlmZmxlL3NlcnZlci9idWZmZXIvU2h1ZmZsZUJ1ZmZlck1hbmFnZXIuamF2YQ==) | `82.14% <76.19%> (-0.62%)` | :arrow_down: |
| [.../org/apache/uniffle/server/ShuffleTaskManager.java](https://codecov.io/gh/apache/incubator-uniffle/pull/471?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2VydmVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS91bmlmZmxlL3NlcnZlci9TaHVmZmxlVGFza01hbmFnZXIuamF2YQ==) | `76.57% <78.57%> (+0.01%)` | :arrow_up: |
| [...a/org/apache/uniffle/server/ShuffleServerConf.java](https://codecov.io/gh/apache/incubator-uniffle/pull/471?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2VydmVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS91bmlmZmxlL3NlcnZlci9TaHVmZmxlU2VydmVyQ29uZi5qYXZh) | `99.31% <100.00%> (+0.02%)` | :arrow_up: |
| [...storage/handler/impl/DataSkippableReadHandler.java](https://codecov.io/gh/apache/incubator-uniffle/pull/471?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==) | `83.78% <0.00%> (-2.71%)` | :arrow_down: |
| [...org/apache/spark/shuffle/writer/AddBlockEvent.java](https://codecov.io/gh/apache/incubator-uniffle/pull/471?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y2xpZW50LXNwYXJrL2NvbW1vbi9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc3Bhcmsvc2h1ZmZsZS93cml0ZXIvQWRkQmxvY2tFdmVudC5qYXZh) | | |
| [...e/spark/shuffle/reader/RssShuffleDataIterator.java](https://codecov.io/gh/apache/incubator-uniffle/pull/471?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y2xpZW50LXNwYXJrL2NvbW1vbi9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc3Bhcmsvc2h1ZmZsZS9yZWFkZXIvUnNzU2h1ZmZsZURhdGFJdGVyYXRvci5qYXZh) | | |
| [...g/apache/hadoop/mapred/SortWriteBufferManager.java](https://codecov.io/gh/apache/incubator-uniffle/pull/471?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y2xpZW50LW1yL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9oYWRvb3AvbWFwcmVkL1NvcnRXcml0ZUJ1ZmZlck1hbmFnZXIuamF2YQ==) | | |
| [...rg/apache/hadoop/mapred/RssMapOutputCollector.java](https://codecov.io/gh/apache/incubator-uniffle/pull/471?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y2xpZW50LW1yL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9oYWRvb3AvbWFwcmVkL1Jzc01hcE91dHB1dENvbGxlY3Rvci5qYXZh) | | |
| [...k/shuffle/writer/WrappedByteArrayOutputStream.java](https://codecov.io/gh/apache/incubator-uniffle/pull/471?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y2xpZW50LXNwYXJrL2NvbW1vbi9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc3Bhcmsvc2h1ZmZsZS93cml0ZXIvV3JhcHBlZEJ5dGVBcnJheU91dHB1dFN0cmVhbS5qYXZh) | | |
| ... and [18 more](https://codecov.io/gh/apache/incubator-uniffle/pull/471?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?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] advancedxy commented on a diff in pull request #471: [ISSUE-378][HugePartition][Part-2] Introduce memory usage limit and data flush
Posted by GitBox <gi...@apache.org>.
advancedxy commented on code in PR #471:
URL: https://github.com/apache/incubator-uniffle/pull/471#discussion_r1070964736
##########
server/src/main/java/org/apache/uniffle/server/ShuffleServerConf.java:
##########
@@ -342,6 +342,20 @@ public class ShuffleServerConf extends RssBaseConf {
.noDefaultValue()
.withDescription("The env key to get json source of local storage media provider");
+ public static final ConfigOption<Long> HUGE_PARTITION_SIZE_THRESHOLD = ConfigOptions
+ .key("rss.server.huge-partition.size.threshold")
+ .longType()
+ .defaultValue(20 * 1024 * 1024 * 1024L)
Review Comment:
> Yes. I think I will add a section about huge partition after all subtasks are finished. WDYT?
Sounds good to me. You can create a new issue to tracking the documentation issue or just polish this docs in the last subtask.
> This value depends on the capacity of a single disk. For example, the single disk capacity is 1TB in our internal env, and I found the max size of huge partition in one single disk is 5. So the total size of huge partition in local disk is 100g (10%)，this is an acceptable config value.
This part will be described more after all subtasks are finished.
This is helpful, please do include this part when you are updating the documentation.
--
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] zuston commented on pull request #471: [ISSUE-378][HugePartition][Part-2] Introduce memory usage limit and data flush
Posted by GitBox <gi...@apache.org>.
zuston commented on PR #471:
URL: https://github.com/apache/incubator-uniffle/pull/471#issuecomment-1383381964
PTAL @advancedxy. After this PR is merged, I will introduce some metrics about huge partition.
--
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] advancedxy commented on a diff in pull request #471: [ISSUE-378][HugePartition][Part-2] Introduce memory usage limit and data flush
Posted by GitBox <gi...@apache.org>.
advancedxy commented on code in PR #471:
URL: https://github.com/apache/incubator-uniffle/pull/471#discussion_r1070859305
##########
server/src/main/java/org/apache/uniffle/server/ShuffleTaskManager.java:
##########
@@ -200,7 +199,13 @@ public StatusCode registerShuffle(
public StatusCode cacheShuffleData(
String appId, int shuffleId, boolean isPreAllocated, ShufflePartitionedData spd) {
refreshAppId(appId);
- return shuffleBufferManager.cacheShuffleData(appId, shuffleId, isPreAllocated, spd);
+ return shuffleBufferManager.cacheShuffleData(
+ appId,
+ shuffleId,
+ isPreAllocated,
+ spd,
+ (appIdx, shuffleIdx, partitionIdx) -> getPartitionDataSize(appIdx, shuffleIdx, partitionIdx)
Review Comment:
I left a new a comment. Let's discuss there.
--
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] zuston commented on a diff in pull request #471: [ISSUE-378][HugePartition][Part-2] Introduce memory usage limit and data flush
Posted by GitBox <gi...@apache.org>.
zuston commented on code in PR #471:
URL: https://github.com/apache/incubator-uniffle/pull/471#discussion_r1070987863
##########
server/src/main/java/org/apache/uniffle/server/ShuffleTaskManager.java:
##########
@@ -200,7 +199,13 @@ public StatusCode registerShuffle(
public StatusCode cacheShuffleData(
String appId, int shuffleId, boolean isPreAllocated, ShufflePartitionedData spd) {
refreshAppId(appId);
- return shuffleBufferManager.cacheShuffleData(appId, shuffleId, isPreAllocated, spd);
+ return shuffleBufferManager.cacheShuffleData(
+ appId,
+ shuffleId,
+ isPreAllocated,
+ spd,
+ this::getPartitionDataSize
Review Comment:
PTAL this?
--
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] advancedxy commented on a diff in pull request #471: [ISSUE-378][HugePartition][Part-2] Introduce memory usage limit and data flush
Posted by GitBox <gi...@apache.org>.
advancedxy commented on code in PR #471:
URL: https://github.com/apache/incubator-uniffle/pull/471#discussion_r1069611212
##########
server/src/main/java/org/apache/uniffle/server/ShuffleServerGrpcService.java:
##########
@@ -353,7 +353,22 @@ public void finishShuffle(FinishShuffleRequest req,
@Override
public void requireBuffer(RequireBufferRequest request,
StreamObserver<RequireBufferResponse> responseObserver) {
- long requireBufferId = shuffleServer.getShuffleTaskManager().requireBuffer(request.getRequireSize());
+ String appId = request.getAppId();
+ long requireBufferId;
+ if (appId == null) {
Review Comment:
I believe this might be wrong. The default value for a string field is ""(empty string) according to https://developers.google.com/protocol-buffers/docs/proto3#default doc.
##########
server/src/main/java/org/apache/uniffle/server/ShuffleTaskManager.java:
##########
@@ -200,7 +199,13 @@ public StatusCode registerShuffle(
public StatusCode cacheShuffleData(
String appId, int shuffleId, boolean isPreAllocated, ShufflePartitionedData spd) {
refreshAppId(appId);
- return shuffleBufferManager.cacheShuffleData(appId, shuffleId, isPreAllocated, spd);
+ return shuffleBufferManager.cacheShuffleData(
+ appId,
+ shuffleId,
+ isPreAllocated,
+ spd,
+ (appIdx, shuffleIdx, partitionIdx) -> getPartitionDataSize(appIdx, shuffleIdx, partitionIdx)
Review Comment:
Java prefers to use Method Reference directly? -> `this::getPartitionDataSize`
I have another concern about this, `getPartitionDataSize` refers `shuffleTaskInfos`, which might leaking `shuffleTaskInfos` to `ShuffleBufferManager`, is there any better ways?
##########
server/src/main/java/org/apache/uniffle/server/ShuffleTaskManager.java:
##########
@@ -630,5 +656,4 @@ private void triggerFlush() {
this.shuffleBufferManager.flushIfNecessary();
}
}
-
Review Comment:
unrelated?
--
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] zuston commented on a diff in pull request #471: [ISSUE-378][HugePartition][Part-2] Introduce memory usage limit and data flush
Posted by GitBox <gi...@apache.org>.
zuston commented on code in PR #471:
URL: https://github.com/apache/incubator-uniffle/pull/471#discussion_r1070751900
##########
server/src/main/java/org/apache/uniffle/server/ShuffleTaskManager.java:
##########
@@ -630,5 +656,4 @@ private void triggerFlush() {
this.shuffleBufferManager.flushIfNecessary();
}
}
-
Review Comment:
Yes.
--
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] zuston commented on pull request #471: [ISSUE-378][HugePartition][Part-2] Introduce memory usage limit and data flush
Posted by GitBox <gi...@apache.org>.
zuston commented on PR #471:
URL: https://github.com/apache/incubator-uniffle/pull/471#issuecomment-1380350449
rss.server.huge-partition.memory.limit.ratio is the conf for memory limitation instead of buffer flush threshold
--
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] zuston commented on pull request #471: [ISSUE-378][HugePartition][Part-2] Introduce memory usage limit and data flush
Posted by GitBox <gi...@apache.org>.
zuston commented on PR #471:
URL: https://github.com/apache/incubator-uniffle/pull/471#issuecomment-1380190430
> So, I think we could reuse the rss.server.single.buffer.flush.threshold settings, which is 64MB by default. We don't have to introduce rss.server.huge-partition.memory.limit.ratio ?
Memory limit is required. Usually the huge partition writing speed is fast, and the HDFS flushing speed is slower than writing. If missing memory limit, the regular partitions will be affected.
By the way, this design has been introduced into our internal version, it works well. Before this PR, sometimes one huge partition will make other regular partitions buffer require fail.
--
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] zuston commented on pull request #471: [ISSUE-378][HugePartition][Part-2] Introduce memory usage limit and data flush
Posted by GitBox <gi...@apache.org>.
zuston commented on PR #471:
URL: https://github.com/apache/incubator-uniffle/pull/471#issuecomment-1379751130
> I don't think we should introduce a new configuration to control buffer flush. rss.server.single.buffer.flush.enabled can be used for the huge partition buffer flush purpose. If the huge partition limit is enabled, the single buffer flush could be enabled automatically.
If single buffer flush could be enabled automatically, how to set the flush threshold size? I don't hope the `rss.server.single.buffer.flush.enabled` is enabled for regular partitions, which could be flushed to ssd/hdd directly instead of cold storage. And huge partition could be flushed to HDFS.
> I think these two similar configurations will bring more confusion to end user.
Emm. Yes.
--
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] advancedxy commented on pull request #471: [ISSUE-378][HugePartition][Part-2] Introduce memory usage limit and data flush
Posted by GitBox <gi...@apache.org>.
advancedxy commented on PR #471:
URL: https://github.com/apache/incubator-uniffle/pull/471#issuecomment-1380296359
> > I'm ok to add rss.server.huge-partition.size.threshold setting, which is used to add memory limit. I just think maybe we should reuse rss.server.single.buffer.flush.threshold instead of another rss.server.huge-partition.memory.limit.ratio settings?
>
> This suggestion has been accepted 😁 (Here: [#471 (comment)](https://github.com/apache/incubator-uniffle/pull/471#issuecomment-1379753276)).
>
> Please review the latest code.
en. I will take a look in details by tomorrow. But on the surface, the `rss.server.huge-partition.memory.limit.ratio` setting still existed?
<img width="944" alt="image" src="https://user-images.githubusercontent.com/807537/212071766-b4f6be0a-0106-4332-8f9e-680910815da2.png">
--
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] zuston commented on a diff in pull request #471: [ISSUE-378][HugePartition][Part-2] Introduce memory usage limit and data flush
Posted by GitBox <gi...@apache.org>.
zuston commented on code in PR #471:
URL: https://github.com/apache/incubator-uniffle/pull/471#discussion_r1070871925
##########
server/src/main/java/org/apache/uniffle/server/ShuffleTaskManager.java:
##########
@@ -200,7 +199,13 @@ public StatusCode registerShuffle(
public StatusCode cacheShuffleData(
String appId, int shuffleId, boolean isPreAllocated, ShufflePartitionedData spd) {
refreshAppId(appId);
- return shuffleBufferManager.cacheShuffleData(appId, shuffleId, isPreAllocated, spd);
+ return shuffleBufferManager.cacheShuffleData(
+ appId,
+ shuffleId,
+ isPreAllocated,
+ spd,
+ this::getPartitionDataSize
Review Comment:
If concurrently caching data for one partition, the function passing will be more accurate, because the partitionDataSize passed may be out of date.
--
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] advancedxy commented on pull request #471: [ISSUE-378][HugePartition][Part-2] Introduce memory usage limit and data flush
Posted by GitBox <gi...@apache.org>.
advancedxy commented on PR #471:
URL: https://github.com/apache/incubator-uniffle/pull/471#issuecomment-1379728663
> Introduce independent buffer flush for huge partition, which will make admin flush data from huge partition to HDFS directly when the conf of rss.server.single.buffer.flush.enabled is disable.
I don't think we should introduce a new configuration to control buffer flush. `rss.server.single.buffer.flush.enabled` can be used for the huge partition buffer flush purpose.
--
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] advancedxy commented on pull request #471: [ISSUE-378][HugePartition][Part-2] Introduce memory usage limit and data flush
Posted by GitBox <gi...@apache.org>.
advancedxy commented on PR #471:
URL: https://github.com/apache/incubator-uniffle/pull/471#issuecomment-1379729718
I think these two similar configurations will bring more confusion to end user.
--
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] advancedxy commented on a diff in pull request #471: [ISSUE-378][HugePartition][Part-2] Introduce memory usage limit and data flush
Posted by GitBox <gi...@apache.org>.
advancedxy commented on code in PR #471:
URL: https://github.com/apache/incubator-uniffle/pull/471#discussion_r1071188864
##########
server/src/main/java/org/apache/uniffle/server/ShuffleTaskManager.java:
##########
@@ -200,7 +199,13 @@ public StatusCode registerShuffle(
public StatusCode cacheShuffleData(
String appId, int shuffleId, boolean isPreAllocated, ShufflePartitionedData spd) {
refreshAppId(appId);
- return shuffleBufferManager.cacheShuffleData(appId, shuffleId, isPreAllocated, spd);
+ return shuffleBufferManager.cacheShuffleData(
+ appId,
+ shuffleId,
+ isPreAllocated,
+ spd,
+ this::getPartitionDataSize
Review Comment:
> If concurrently caching data for one partition, the function passing will be more accurate, because the partitionDataSize passed may be out of date.
Yes. it would be more accurate. But I'm not sure it's worthing it.
However I'm going to approve this for now. Let's see how it works in prod. Maybe we need to modify it later.
--
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] zuston merged pull request #471: [ISSUE-378][HugePartition][Part-2] Introduce memory usage limit and data flush
Posted by GitBox <gi...@apache.org>.
zuston merged PR #471:
URL: https://github.com/apache/incubator-uniffle/pull/471
--
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] zuston commented on a diff in pull request #471: [ISSUE-378][HugePartition][Part-2] Introduce memory usage limit and data flush
Posted by GitBox <gi...@apache.org>.
zuston commented on code in PR #471:
URL: https://github.com/apache/incubator-uniffle/pull/471#discussion_r1070753360
##########
server/src/main/java/org/apache/uniffle/server/ShuffleTaskManager.java:
##########
@@ -200,7 +199,13 @@ public StatusCode registerShuffle(
public StatusCode cacheShuffleData(
String appId, int shuffleId, boolean isPreAllocated, ShufflePartitionedData spd) {
refreshAppId(appId);
- return shuffleBufferManager.cacheShuffleData(appId, shuffleId, isPreAllocated, spd);
+ return shuffleBufferManager.cacheShuffleData(
+ appId,
+ shuffleId,
+ isPreAllocated,
+ spd,
+ (appIdx, shuffleIdx, partitionIdx) -> getPartitionDataSize(appIdx, shuffleIdx, partitionIdx)
Review Comment:
> Java prefers to use Method Reference directly? -> this::getPartitionDataSize
Got it.
> I have another concern about this, getPartitionDataSize refers shuffleTaskInfos, which might leaking shuffleTaskInfos to ShuffleBufferManager, is there any better ways?
Emm... I have no ideas
--
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] zuston commented on a diff in pull request #471: [ISSUE-378][HugePartition][Part-2] Introduce memory usage limit and data flush
Posted by GitBox <gi...@apache.org>.
zuston commented on code in PR #471:
URL: https://github.com/apache/incubator-uniffle/pull/471#discussion_r1070987626
##########
server/src/main/java/org/apache/uniffle/server/ShuffleServerConf.java:
##########
@@ -342,6 +342,20 @@ public class ShuffleServerConf extends RssBaseConf {
.noDefaultValue()
.withDescription("The env key to get json source of local storage media provider");
+ public static final ConfigOption<Long> HUGE_PARTITION_SIZE_THRESHOLD = ConfigOptions
+ .key("rss.server.huge-partition.size.threshold")
+ .longType()
+ .defaultValue(20 * 1024 * 1024 * 1024L)
Review Comment:
Yes. I will.
--
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] zuston commented on a diff in pull request #471: [ISSUE-378][HugePartition][Part-2] Introduce memory usage limit and data flush
Posted by GitBox <gi...@apache.org>.
zuston commented on code in PR #471:
URL: https://github.com/apache/incubator-uniffle/pull/471#discussion_r1070868831
##########
server/src/main/java/org/apache/uniffle/server/ShuffleServerConf.java:
##########
@@ -342,6 +342,20 @@ public class ShuffleServerConf extends RssBaseConf {
.noDefaultValue()
.withDescription("The env key to get json source of local storage media provider");
+ public static final ConfigOption<Long> HUGE_PARTITION_SIZE_THRESHOLD = ConfigOptions
+ .key("rss.server.huge-partition.size.threshold")
+ .longType()
+ .defaultValue(20 * 1024 * 1024 * 1024L)
Review Comment:
> Is 20GB has some meaningful background?
This value depends on the capacity of a single disk. For example, the single disk capacity is 1TB in our internal env, and I found the max size of huge partition in one single disk is 5. So the total size of huge partition in local disk is 100g (10%)，this is an acceptable config value.
This part will be described more after all subtasks are finished.
> It would be great if you could add some suggestion values in the doc description.
Also could you please update the configuration doc?
Yes. I think I will add a section about huge partition after all subtasks are finished. WDYT?
--
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] advancedxy commented on a diff in pull request #471: [ISSUE-378][HugePartition][Part-2] Introduce memory usage limit and data flush
Posted by GitBox <gi...@apache.org>.
advancedxy commented on code in PR #471:
URL: https://github.com/apache/incubator-uniffle/pull/471#discussion_r1070964736
##########
server/src/main/java/org/apache/uniffle/server/ShuffleServerConf.java:
##########
@@ -342,6 +342,20 @@ public class ShuffleServerConf extends RssBaseConf {
.noDefaultValue()
.withDescription("The env key to get json source of local storage media provider");
+ public static final ConfigOption<Long> HUGE_PARTITION_SIZE_THRESHOLD = ConfigOptions
+ .key("rss.server.huge-partition.size.threshold")
+ .longType()
+ .defaultValue(20 * 1024 * 1024 * 1024L)
Review Comment:
> Yes. I think I will add a section about huge partition after all subtasks are finished. WDYT?
Sounds good to me. You can create a new issue to tracking the documentation issue or just polish this docs in the last subtask.
> This value depends on the capacity of a single disk. For example, the single disk capacity is 1TB in our internal env, and I found the max size of huge partition in one single disk is 5. So the total size of huge partition in local disk is 100g (10%)，this is an acceptable config value.
This part will be described more after all subtasks are finished.
This is helpful, please do include this part when you are updating the documentaion.
--
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] advancedxy commented on pull request #471: [ISSUE-378][HugePartition][Part-2] Introduce memory usage limit and data flush
Posted by GitBox <gi...@apache.org>.
advancedxy commented on PR #471:
URL: https://github.com/apache/incubator-uniffle/pull/471#issuecomment-1380242176
> > So, I think we could reuse the rss.server.single.buffer.flush.threshold settings, which is 64MB by default. We don't have to introduce rss.server.huge-partition.memory.limit.ratio ?
>
> Memory limit is required. Usually the huge partition writing speed is fast, and the HDFS flushing speed is slower than writing. If missing memory limit, the regular partitions will be affected.
>
> By the way, this design has been introduced into our internal version, it works well. Before this PR, sometimes one huge partition will make other regular partitions buffer require fail.
I'm ok to add `rss.server.huge-partition.size.threshold` setting, which is used to add memory limit. I just think maybe we should reuse `rss.server.single.buffer.flush.threshold` instead of another `rss.server.huge-partition.memory.limit.ratio` settings?
--
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] advancedxy commented on pull request #471: [ISSUE-378][HugePartition][Part-2] Introduce memory usage limit and data flush
Posted by GitBox <gi...@apache.org>.
advancedxy commented on PR #471:
URL: https://github.com/apache/incubator-uniffle/pull/471#issuecomment-1380179474
> > I don't think we should introduce a new configuration to control buffer flush. rss.server.single.buffer.flush.enabled can be used for the huge partition buffer flush purpose. If the huge partition limit is enabled, the single buffer flush could be enabled automatically.
>
> If single buffer flush could be enabled automatically, how to set the flush threshold size? I don't hope the `rss.server.single.buffer.flush.enabled` is enabled for regular partitions, which could be flushed to ssd/hdd directly instead of cold storage. And huge partition could be flushed to HDFS.
>
> > I think these two similar configurations will bring more confusion to end user.
>
> Emm. Yes.
Sorry, I forgot to reply this comment.
If I understand the code and design correctly, `single.buffer.flushed.enabled` is added to support flushing big buffer to cold storage, such as hdfs directly, which I think it perfectly matches you intention when serving huge partitions.
So, I think we could reuse the `rss.server.single.buffer.flush.threshold` settings, which is 64MB by default. We don't have to introduce `rss.server.huge-partition.memory.limit.ratio` ?
--
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