You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@celeborn.apache.org by "JQ-Cao (via GitHub)" <gi...@apache.org> on 2023/07/10 12:25:52 UTC
[GitHub] [incubator-celeborn] JQ-Cao opened a new pull request, #1701: [CELEBORN-785] Add a configuration to ensure that, in soft mode, indi…
JQ-Cao opened a new pull request, #1701:
URL: https://github.com/apache/incubator-celeborn/pull/1701
### What changes were proposed in this pull request?
Add a configuration `celeborn.worker.shuffle.partitionSplit.max` to ensure that, in soft mode, individual partition files are limited to a size smaller than the configured value
### Why are the changes needed?
In soft mode, there may be situations where individual partition files are exceptionally large, which can result in excessively long sort times in skewed scenarios.
### Does this PR introduce _any_ user-facing change?
`celeborn.worker.shuffle.partitionSplit.max` defalut value 16g
### How was this patch tested?
none
--
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
[GitHub] [incubator-celeborn] waitinfuture closed pull request #1701: [CELEBORN-785] Add worker side partition hard split threshold
Posted by "waitinfuture (via GitHub)" <gi...@apache.org>.
waitinfuture closed pull request #1701: [CELEBORN-785] Add worker side partition hard split threshold
URL: https://github.com/apache/incubator-celeborn/pull/1701
--
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
[GitHub] [incubator-celeborn] JQ-Cao commented on a diff in pull request #1701: [CELEBORN-785] Add a configuration to ensure that, in soft mode, indi…
Posted by "JQ-Cao (via GitHub)" <gi...@apache.org>.
JQ-Cao commented on code in PR #1701:
URL: https://github.com/apache/incubator-celeborn/pull/1701#discussion_r1259116894
##########
common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala:
##########
@@ -1930,6 +1931,15 @@ object CelebornConf extends Logging {
.bytesConf(ByteUnit.BYTE)
.createWithDefaultString("1m")
+ val WORKER_PARTITION_SPLIT_MAX_SIZE: ConfigEntry[Long] =
+ buildConf("celeborn.worker.shuffle.partitionSplit.max")
+ .withAlternative("celeborn.shuffle.partitionSplit.max")
Review Comment:
ok, i remove it
--
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
[GitHub] [incubator-celeborn] pan3793 commented on a diff in pull request #1701: [CELEBORN-785] Add a configuration to ensure that, in soft mode, indi…
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #1701:
URL: https://github.com/apache/incubator-celeborn/pull/1701#discussion_r1259083530
##########
common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala:
##########
@@ -1930,6 +1931,15 @@ object CelebornConf extends Logging {
.bytesConf(ByteUnit.BYTE)
.createWithDefaultString("1m")
+ val WORKER_PARTITION_SPLIT_MAX_SIZE: ConfigEntry[Long] =
+ buildConf("celeborn.worker.shuffle.partitionSplit.max")
+ .withAlternative("celeborn.shuffle.partitionSplit.max")
+ .categories("worker")
+ .doc("Specify the maximum partition size for splitting, and ensure that individual partition files are always smaller than this limit.")
+ .version("0.3.0")
+ .bytesConf(ByteUnit.BYTE)
+ .createWithDefaultString("16g")
Review Comment:
is this too big? I know your case the data is extremely huge, but the default value should be based on the common cases.
--
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
[GitHub] [incubator-celeborn] codecov[bot] commented on pull request #1701: [CELEBORN-785] Add a configuration to ensure that, in soft mode, indi…
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #1701:
URL: https://github.com/apache/incubator-celeborn/pull/1701#issuecomment-1630065258
## [Codecov](https://app.codecov.io/gh/apache/incubator-celeborn/pull/1701?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#1701](https://app.codecov.io/gh/apache/incubator-celeborn/pull/1701?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (84ad14a) into [main](https://app.codecov.io/gh/apache/incubator-celeborn/commit/a7ff28c26d9701ae3958513bdcc2b42df1a4c0af?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (a7ff28c) will **increase** coverage by `0.21%`.
> The diff coverage is `87.50%`.
```diff
@@ Coverage Diff @@
## main #1701 +/- ##
==========================================
+ Coverage 46.41% 46.62% +0.21%
==========================================
Files 161 161
Lines 10011 10026 +15
Branches 928 928
==========================================
+ Hits 4646 4674 +28
+ Misses 5050 5039 -11
+ Partials 315 313 -2
```
| [Impacted Files](https://app.codecov.io/gh/apache/incubator-celeborn/pull/1701?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [...cala/org/apache/celeborn/common/CelebornConf.scala](https://app.codecov.io/gh/apache/incubator-celeborn/pull/1701?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29tbW9uL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvY2VsZWJvcm4vY29tbW9uL0NlbGVib3JuQ29uZi5zY2FsYQ==) | `87.23% <87.50%> (+0.04%)` | :arrow_up: |
... and [2 files with indirect coverage changes](https://app.codecov.io/gh/apache/incubator-celeborn/pull/1701/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
: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=apache)
--
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
[GitHub] [incubator-celeborn] AngersZhuuuu commented on a diff in pull request #1701: [CELEBORN-785] Add worker side partition hard split threshold
Posted by "AngersZhuuuu (via GitHub)" <gi...@apache.org>.
AngersZhuuuu commented on code in PR #1701:
URL: https://github.com/apache/incubator-celeborn/pull/1701#discussion_r1259237595
##########
common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala:
##########
@@ -1930,6 +1931,14 @@ object CelebornConf extends Logging {
.bytesConf(ByteUnit.BYTE)
.createWithDefaultString("1m")
+ val WORKER_PARTITION_SPLIT_MAX_SIZE: ConfigEntry[Long] =
+ buildConf("celeborn.worker.shuffle.partitionSplit.max")
+ .categories("worker")
+ .doc("Specify the maximum partition size for splitting, and ensure that individual partition files are always smaller than this limit.")
+ .version("0.3.0")
+ .bytesConf(ByteUnit.BYTE)
+ .createWithDefaultString("2g")
Review Comment:
Should we add a check value here to make sure max bigger than min? cc @pan3793 @waitinfuture @JQ-Cao
--
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
[GitHub] [incubator-celeborn] waitinfuture commented on a diff in pull request #1701: [CELEBORN-785] Add a configuration to ensure that, in soft mode, indi…
Posted by "waitinfuture (via GitHub)" <gi...@apache.org>.
waitinfuture commented on code in PR #1701:
URL: https://github.com/apache/incubator-celeborn/pull/1701#discussion_r1258574096
##########
common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala:
##########
@@ -1930,6 +1931,15 @@ object CelebornConf extends Logging {
.bytesConf(ByteUnit.BYTE)
.createWithDefaultString("1m")
+ val WORKER_PARTITION_SPLIT_MAX_SIZE: ConfigEntry[Long] =
+ buildConf("celeborn.worker.shuffle.partitionSplit.max")
+ .withAlternative("celeborn.shuffle.partitionSplit.max")
Review Comment:
I think we don't need ```withAlternative``` here, it's only used for backward compatibility.
--
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
[GitHub] [incubator-celeborn] zhongqiangczq commented on pull request #1701: [CELEBORN-785] Add a configuration to ensure that, in soft mode, indi…
Posted by "zhongqiangczq (via GitHub)" <gi...@apache.org>.
zhongqiangczq commented on PR #1701:
URL: https://github.com/apache/incubator-celeborn/pull/1701#issuecomment-1628919939
Celeborn CI has errors. you can fix by running command `UPDATE=1 build/mvn clean test -pl common -am -Pspark-3.3 -Dtest=none -DwildcardSuites=org.apache.celeborn.ConfigurationSuite`
--
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
[GitHub] [incubator-celeborn] JQ-Cao commented on pull request #1701: [CELEBORN-785] Add a configuration to ensure that, in soft mode, indi…
Posted by "JQ-Cao (via GitHub)" <gi...@apache.org>.
JQ-Cao commented on PR #1701:
URL: https://github.com/apache/incubator-celeborn/pull/1701#issuecomment-1630032406
>
> Would it be possible to add a unit test to validate that the newly added configuration functions as intended?
This is a simple logic and unit testing may be difficult to trigger. However, since we are already using this logic in production, I'm trying to take a shortcut
--
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
[GitHub] [incubator-celeborn] JQ-Cao commented on a diff in pull request #1701: [CELEBORN-785] Add worker side partition hard split threshold
Posted by "JQ-Cao (via GitHub)" <gi...@apache.org>.
JQ-Cao commented on code in PR #1701:
URL: https://github.com/apache/incubator-celeborn/pull/1701#discussion_r1259181984
##########
common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala:
##########
@@ -1930,6 +1931,15 @@ object CelebornConf extends Logging {
.bytesConf(ByteUnit.BYTE)
.createWithDefaultString("1m")
+ val WORKER_PARTITION_SPLIT_MAX_SIZE: ConfigEntry[Long] =
+ buildConf("celeborn.worker.shuffle.partitionSplit.max")
+ .withAlternative("celeborn.shuffle.partitionSplit.max")
+ .categories("worker")
+ .doc("Specify the maximum partition size for splitting, and ensure that individual partition files are always smaller than this limit.")
+ .version("0.3.0")
+ .bytesConf(ByteUnit.BYTE)
+ .createWithDefaultString("16g")
Review Comment:
Okay, I'm done with the changes
--
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
[GitHub] [incubator-celeborn] pan3793 commented on a diff in pull request #1701: [CELEBORN-785] Add a configuration to ensure that, in soft mode, indi…
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #1701:
URL: https://github.com/apache/incubator-celeborn/pull/1701#discussion_r1259154450
##########
common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala:
##########
@@ -1930,6 +1931,15 @@ object CelebornConf extends Logging {
.bytesConf(ByteUnit.BYTE)
.createWithDefaultString("1m")
+ val WORKER_PARTITION_SPLIT_MAX_SIZE: ConfigEntry[Long] =
+ buildConf("celeborn.worker.shuffle.partitionSplit.max")
+ .withAlternative("celeborn.shuffle.partitionSplit.max")
+ .categories("worker")
+ .doc("Specify the maximum partition size for splitting, and ensure that individual partition files are always smaller than this limit.")
+ .version("0.3.0")
+ .bytesConf(ByteUnit.BYTE)
+ .createWithDefaultString("16g")
Review Comment:
Given the default value of `celeborn.client.shuffle.partitionSplit.threshold` is 1G, in the normal case, the real split partition size should be a little larger than 1G, set it to 2G as an HRAD_SPLIT threshold should be fine. Otherwise, users may be surprised if they see an extremely large file than they wanted.
--
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
[GitHub] [incubator-celeborn] cfmcgrady commented on pull request #1701: [CELEBORN-785] Add a configuration to ensure that, in soft mode, indi…
Posted by "cfmcgrady (via GitHub)" <gi...@apache.org>.
cfmcgrady commented on PR #1701:
URL: https://github.com/apache/incubator-celeborn/pull/1701#issuecomment-1629493289
Would it be possible to add a unit test to validate that the newly added configuration functions as intended?
--
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
[GitHub] [incubator-celeborn] JQ-Cao commented on pull request #1701: [CELEBORN-785] Add a configuration to ensure that, in soft mode, indi…
Posted by "JQ-Cao (via GitHub)" <gi...@apache.org>.
JQ-Cao commented on PR #1701:
URL: https://github.com/apache/incubator-celeborn/pull/1701#issuecomment-1628948962
> Celeborn CI has errors. you can fix by running command `UPDATE=1 build/mvn clean test -pl common -am -Pspark-3.3 -Dtest=none -DwildcardSuites=org.apache.celeborn.ConfigurationSuite` 
done
--
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
[GitHub] [incubator-celeborn] pan3793 commented on a diff in pull request #1701: [CELEBORN-785] Add a configuration to ensure that, in soft mode, indi…
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #1701:
URL: https://github.com/apache/incubator-celeborn/pull/1701#discussion_r1259082930
##########
common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala:
##########
@@ -1930,6 +1931,15 @@ object CelebornConf extends Logging {
.bytesConf(ByteUnit.BYTE)
.createWithDefaultString("1m")
+ val WORKER_PARTITION_SPLIT_MAX_SIZE: ConfigEntry[Long] =
+ buildConf("celeborn.worker.shuffle.partitionSplit.max")
+ .withAlternative("celeborn.shuffle.partitionSplit.max")
Review Comment:
`withAlternative` is the alias, we don't need it for newly added 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
[GitHub] [incubator-celeborn] JQ-Cao commented on a diff in pull request #1701: [CELEBORN-785] Add a configuration to ensure that, in soft mode, indi…
Posted by "JQ-Cao (via GitHub)" <gi...@apache.org>.
JQ-Cao commented on code in PR #1701:
URL: https://github.com/apache/incubator-celeborn/pull/1701#discussion_r1259117840
##########
common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala:
##########
@@ -1930,6 +1931,15 @@ object CelebornConf extends Logging {
.bytesConf(ByteUnit.BYTE)
.createWithDefaultString("1m")
+ val WORKER_PARTITION_SPLIT_MAX_SIZE: ConfigEntry[Long] =
+ buildConf("celeborn.worker.shuffle.partitionSplit.max")
+ .withAlternative("celeborn.shuffle.partitionSplit.max")
+ .categories("worker")
+ .doc("Specify the maximum partition size for splitting, and ensure that individual partition files are always smaller than this limit.")
+ .version("0.3.0")
+ .bytesConf(ByteUnit.BYTE)
+ .createWithDefaultString("16g")
Review Comment:
On our production, 4GB is a set value, but it is only used to limit the file size in worst-case scenarios, so a very large value is given. In typical scenarios, this value should not be set.
--
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