You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@celeborn.apache.org by "FMX (via GitHub)" <gi...@apache.org> on 2024/10/10 07:30:33 UTC
[PR] [CELEBORN-1637] Add a config to bypass memory check on sorting files [celeborn]
FMX opened a new pull request, #2798:
URL: https://github.com/apache/celeborn/pull/2798
### What changes were proposed in this pull request?
Add a config to bypass memory check when sorting shuffle files.
### Why are the changes needed?
If a celeborn worker has quite a large memory and it supports both Spark and Flink engines. This config should be enabled.
### Does this PR introduce _any_ user-facing change?
NO.
### How was this patch tested?
Cluster test.
--
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@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1637] Enhance config to bypass memory check for partition file sorter [celeborn]
Posted by "FMX (via GitHub)" <gi...@apache.org>.
FMX commented on PR #2798:
URL: https://github.com/apache/celeborn/pull/2798#issuecomment-2404560019
> > The memory manager needs evolvements which I'll come up with a CIP later.
>
> I just saw this comment. For this case, I think we should classify configurations to each impl, for example
>
> ```
> celeborn.worker.partitionSorter.memoryManager=none|abc|xyz
> celeborn.worker.partitionSorter.abc.directMemoryRatioThreshold
> celeborn.worker.partitionSorter.abc.otherProperty
> celeborn.worker.partitionSorter.xyz.property1
> celeborn.worker.partitionSorter.xyz.property2
> ```
>
> then `celeborn.worker.partitionSorter.memoryManager=none` means disabling memory check. please correct me if I understand your proposal wrong.
No, I mean that I want to track detailed memory usage. Evolving the memory to support fine-grade memory management, I think Flink's memory management can be some kind of reference, but skip its credit-based mechanism.
--
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@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1637] Add a config to bypass memory check on sorting files [celeborn]
Posted by "s0nskar (via GitHub)" <gi...@apache.org>.
s0nskar commented on PR #2798:
URL: https://github.com/apache/celeborn/pull/2798#issuecomment-2404435939
Got it, thanks for the explanation @FMX @SteNicholas
--
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@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1637] Add a config to bypass memory check on sorting files [celeborn]
Posted by "SteNicholas (via GitHub)" <gi...@apache.org>.
SteNicholas commented on PR #2798:
URL: https://github.com/apache/celeborn/pull/2798#issuecomment-2404365984
> directMemoryRatioThreshold
@s0nskar, this skips the memory check to continue partition sorting if a celeborn worker has quite a large memory, no need to wait memory ready.
--
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@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1637] Add a config to bypass memory check on sorting files [celeborn]
Posted by "s0nskar (via GitHub)" <gi...@apache.org>.
s0nskar commented on PR #2798:
URL: https://github.com/apache/celeborn/pull/2798#issuecomment-2404334077
q: Wouldn't this be same as setting `directMemoryRatioThreshold` to 1?
--
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@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1637] Add a config to bypass memory check on sorting files [celeborn]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #2798:
URL: https://github.com/apache/celeborn/pull/2798#discussion_r1794978993
##########
docs/configuration/worker.md:
##########
@@ -161,6 +161,7 @@ license: |
| celeborn.worker.shuffle.partitionSplit.min | 1m | false | Min size for a partition to split | 0.3.0 | celeborn.shuffle.partitionSplit.min |
| celeborn.worker.sortPartition.indexCache.expire | 180s | false | PartitionSorter's cache item expire time. | 0.4.0 | |
| celeborn.worker.sortPartition.indexCache.maxWeight | 100000 | false | PartitionSorter's cache max weight for index buffer. | 0.4.0 | |
+| celeborn.worker.sortPartition.memoryCheck.bypass | false | false | Bypass memory check in partition files sorter. If your worker has quite large direct memory and same Celeborn cluster supports Spark and Flink, enable this setting will be helpful. | 0.6.0 | |
Review Comment:
docs of `celeborn.worker.partitionSorter.directMemoryRatioThreshold` should be updated too.
--
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@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1637] Add a config to bypass memory check on sorting files [celeborn]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on PR #2798:
URL: https://github.com/apache/celeborn/pull/2798#issuecomment-2404524240
> The memory manager needs evolvements which I'll come up with a CIP later.
I just saw this comment. For this case, I think we should classify configurations to each impl, for example
```
celeborn.worker.partitionSorter.memoryManager=none|abc|xyz
celeborn.worker.partitionSorter.abc.directMemoryRatioThreshold
celeborn.worker.partitionSorter.abc.otherProperty
celeborn.worker.partitionSorter.xyz.property1
celeborn.worker.partitionSorter.xyz.property2
```
then `celeborn.worker.partitionSorter.memoryManager=none` means disabling memory check. please correct me if I understand your proposal wrong.
--
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@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1637] Add a config to bypass memory check on sorting files [celeborn]
Posted by "FMX (via GitHub)" <gi...@apache.org>.
FMX commented on PR #2798:
URL: https://github.com/apache/celeborn/pull/2798#issuecomment-2404415620
> directMemoryRatioThreshold
```java
public boolean sortMemoryReady() {
return currentServingState() == ServingState.NONE_PAUSED
&& sortMemoryCounter.get() < maxSortMemory;
}
```
No, it's different. The purpose of this config is to bypass the check of ServingState. This PR can solve a problem when a Celeborn cluster is used for both Spark and Flink. The memory manager needs evolvements which I'll come up with a CIP 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@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1637] Add a config to bypass memory check on sorting files [celeborn]
Posted by "SteNicholas (via GitHub)" <gi...@apache.org>.
SteNicholas commented on PR #2798:
URL: https://github.com/apache/celeborn/pull/2798#issuecomment-2404367517
> q: Wouldn't this be same as setting `directMemoryRatioThreshold` to 1?
@s0nskar, this config option is only used to skip the memory check to continue partition sorting if a celeborn worker has quite a large memory, no need to wait memory ready.
--
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@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1637] Add a config to bypass memory check on sorting files [celeborn]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #2798:
URL: https://github.com/apache/celeborn/pull/2798#discussion_r1794996405
##########
docs/configuration/worker.md:
##########
@@ -161,6 +161,7 @@ license: |
| celeborn.worker.shuffle.partitionSplit.min | 1m | false | Min size for a partition to split | 0.3.0 | celeborn.shuffle.partitionSplit.min |
| celeborn.worker.sortPartition.indexCache.expire | 180s | false | PartitionSorter's cache item expire time. | 0.4.0 | |
| celeborn.worker.sortPartition.indexCache.maxWeight | 100000 | false | PartitionSorter's cache max weight for index buffer. | 0.4.0 | |
+| celeborn.worker.sortPartition.memoryCheck.bypass | false | false | Bypass memory check in partition files sorter. If your worker has quite large direct memory and same Celeborn cluster supports Spark and Flink, enable this setting will be helpful. | 0.6.0 | |
Review Comment:
seems we can simply treat zero value for `celeborn.worker.partitionSorter.directMemoryRatioThreshold` as disable memory check for partition sorter, to avoid introducing another configuration.
--
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@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1637] Add a config to bypass memory check on sorting files [celeborn]
Posted by "FMX (via GitHub)" <gi...@apache.org>.
FMX commented on code in PR #2798:
URL: https://github.com/apache/celeborn/pull/2798#discussion_r1795002643
##########
docs/configuration/worker.md:
##########
@@ -161,6 +161,7 @@ license: |
| celeborn.worker.shuffle.partitionSplit.min | 1m | false | Min size for a partition to split | 0.3.0 | celeborn.shuffle.partitionSplit.min |
| celeborn.worker.sortPartition.indexCache.expire | 180s | false | PartitionSorter's cache item expire time. | 0.4.0 | |
| celeborn.worker.sortPartition.indexCache.maxWeight | 100000 | false | PartitionSorter's cache max weight for index buffer. | 0.4.0 | |
+| celeborn.worker.sortPartition.memoryCheck.bypass | false | false | Bypass memory check in partition files sorter. If your worker has quite large direct memory and same Celeborn cluster supports Spark and Flink, enable this setting will be helpful. | 0.6.0 | |
Review Comment:
Sounds good. I'll change this PR.
--
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@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1637] Enhance config to bypass memory check for partition file sorter [celeborn]
Posted by "SteNicholas (via GitHub)" <gi...@apache.org>.
SteNicholas closed pull request #2798: [CELEBORN-1637] Enhance config to bypass memory check for partition file sorter
URL: https://github.com/apache/celeborn/pull/2798
--
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@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org