You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@celeborn.apache.org by "leixm (via GitHub)" <gi...@apache.org> on 2024/10/10 09:03:49 UTC
[PR] [WIP][CELEBORN-1577][Phase1] Storage quota should support interrupt shuffle. [celeborn]
leixm opened a new pull request, #2801:
URL: https://github.com/apache/celeborn/pull/2801
### What changes were proposed in this pull request?
Support interrupt shuffle on client side.
I will develop the following functions in order
1. Client supports interrupt shuffle
2. Master supports calculating app-level shuffle usage
### Why are the changes needed?
The current storage quota logic can only limit new shuffles, and cannot limit the writing of existing shuffles. In our production environment, there is such an scenario: the cluster is small, but the user's app single shuffle is large which occupied disk resources, we want to interrupt those shuffle.
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
Unable to test this part independently, Additional tests will be added after completing the second part.
--
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-1577][Phase1] Storage quota should support interrupt shuffle. [celeborn]
Posted by "RexXiong (via GitHub)" <gi...@apache.org>.
RexXiong commented on code in PR #2801:
URL: https://github.com/apache/celeborn/pull/2801#discussion_r1806072593
##########
master/src/main/scala/org/apache/celeborn/service/deploy/master/Master.scala:
##########
@@ -1105,7 +1105,8 @@ private[celeborn] class Master(
(statusSystem.excludedWorkers.asScala ++ statusSystem.manuallyExcludedWorkers.asScala).asJava),
needCheckedWorkerList,
new util.ArrayList[WorkerInfo](
- (statusSystem.shutdownWorkers.asScala ++ statusSystem.decommissionWorkers.asScala).asJava)))
+ (statusSystem.shutdownWorkers.asScala ++ statusSystem.decommissionWorkers.asScala).asJava),
+ CheckQuotaResponse(isAvailable = true, "")))
Review Comment:
should actually check quota for the application.
--
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-1577][Phase1] Storage quota should support interrupt shuffle. [celeborn]
Posted by "RexXiong (via GitHub)" <gi...@apache.org>.
RexXiong commented on code in PR #2801:
URL: https://github.com/apache/celeborn/pull/2801#discussion_r1811997290
##########
master/src/main/scala/org/apache/celeborn/service/deploy/master/Master.scala:
##########
@@ -1105,7 +1105,8 @@ private[celeborn] class Master(
(statusSystem.excludedWorkers.asScala ++ statusSystem.manuallyExcludedWorkers.asScala).asJava),
needCheckedWorkerList,
new util.ArrayList[WorkerInfo](
- (statusSystem.shutdownWorkers.asScala ++ statusSystem.decommissionWorkers.asScala).asJava)))
+ (statusSystem.shutdownWorkers.asScala ++ statusSystem.decommissionWorkers.asScala).asJava),
+ CheckQuotaResponse(isAvailable = true, "")))
Review Comment:
Ok
--
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] [WIP][CELEBORN-1577][Phase1] Storage quota should support interrupt shuffle. [celeborn]
Posted by "leixm (via GitHub)" <gi...@apache.org>.
leixm commented on PR #2801:
URL: https://github.com/apache/celeborn/pull/2801#issuecomment-2404711990
@AngersZhuuuu @RexXiong Can you help review?
--
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-1577][Phase1] Storage quota should support interrupt shuffle. [celeborn]
Posted by "RexXiong (via GitHub)" <gi...@apache.org>.
RexXiong closed pull request #2801: [CELEBORN-1577][Phase1] Storage quota should support interrupt shuffle.
URL: https://github.com/apache/celeborn/pull/2801
--
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-1577][Phase1] Storage quota should support interrupt shuffle. [celeborn]
Posted by "leixm (via GitHub)" <gi...@apache.org>.
leixm commented on PR #2801:
URL: https://github.com/apache/celeborn/pull/2801#issuecomment-2441253164
cc @RexXiong Done, thanks for your review.
--
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-1577][Phase1] Storage quota should support interrupt shuffle. [celeborn]
Posted by "RexXiong (via GitHub)" <gi...@apache.org>.
RexXiong commented on code in PR #2801:
URL: https://github.com/apache/celeborn/pull/2801#discussion_r1818282821
##########
common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala:
##########
@@ -1334,6 +1334,7 @@ class CelebornConf(loadDefaults: Boolean) extends Cloneable with Logging with Se
def clientShuffleMapPartitionSplitEnabled = get(CLIENT_SHUFFLE_MAPPARTITION_SPLIT_ENABLED)
def clientChunkPrefetchEnabled = get(CLIENT_CHUNK_PREFETCH_ENABLED)
def clientInputStreamCreationWindow = get(CLIENT_INPUTSTREAM_CREATION_WINDOW)
+ def clientQuotaInterruptShuffleEnable = get(CLIENT_QUOTA_INTERRUPT_SHUFFLE_ENABLED)
Review Comment:
clientQuotaInterruptShuffleEnable -》 clientQuotaInterruptShuffleEnabled
##########
common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala:
##########
@@ -5899,4 +5900,12 @@ object CelebornConf extends Logging {
.doubleConf
.checkValue(v => v > 0.0 && v <= 1.0, "Should be in (0.0, 1.0].")
.createWithDefault(1)
+
+ val CLIENT_QUOTA_INTERRUPT_SHUFFLE_ENABLED: ConfigEntry[Boolean] =
+ buildConf("celeborn.client.quota.interruptShuffle.enabled")
Review Comment:
Perhaps we should rename the configuration to celeborn.quota.interruptShuffle.enabled, as 'quota' represents an independent configuration namespace. It would also be appropriate to move this setting to the quota section.
##########
client/src/main/scala/org/apache/celeborn/client/LifecycleManager.scala:
##########
@@ -54,7 +54,6 @@ import org.apache.celeborn.common.rpc.{ClientSaslContextBuilder, RpcSecurityCont
import org.apache.celeborn.common.rpc.netty.{LocalNettyRpcCallContext, RemoteNettyRpcCallContext}
import org.apache.celeborn.common.util.{JavaUtils, PbSerDeUtils, ThreadUtils, Utils}
// Can Remove this if celeborn don't support scala211 in future
Review Comment:
Don't remove 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@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1577][Phase1] Storage quota should support interrupt shuffle. [celeborn]
Posted by "RexXiong (via GitHub)" <gi...@apache.org>.
RexXiong commented on code in PR #2801:
URL: https://github.com/apache/celeborn/pull/2801#discussion_r1799101551
##########
client/src/main/scala/org/apache/celeborn/client/ApplicationHeartbeater.scala:
##########
@@ -68,6 +69,7 @@ class ApplicationHeartbeater(
if (response.statusCode == StatusCode.SUCCESS) {
logDebug("Successfully send app heartbeat.")
workerStatusTracker.handleHeartbeatResponse(response)
+ checkQuotaExceeds(response.checkQuotaResponse)
Review Comment:
Maybe add a flag to tell spark whether cancel the stage when quota exceeds?
--
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-1577][Phase1] Storage quota should support interrupt shuffle. [celeborn]
Posted by "leixm (via GitHub)" <gi...@apache.org>.
leixm commented on PR #2801:
URL: https://github.com/apache/celeborn/pull/2801#issuecomment-2436660857
cc @RexXiong PTAL.
--
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-1577][Phase1] Storage quota should support interrupt shuffle. [celeborn]
Posted by "RexXiong (via GitHub)" <gi...@apache.org>.
RexXiong commented on code in PR #2801:
URL: https://github.com/apache/celeborn/pull/2801#discussion_r1821000773
##########
common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala:
##########
@@ -5333,6 +5334,14 @@ object CelebornConf extends Logging {
.longConf
.createWithDefault(Long.MaxValue)
+ val QUOTA_INTERRUPT_SHUFFLE_ENABLED: ConfigEntry[Boolean] =
+ buildConf("celeborn.quota.interruptShuffle.enabled")
+ .categories("quota", "master", "client")
Review Comment:
seems this configuration never used at master
--
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-1577][Phase1] Storage quota should support interrupt shuffle. [celeborn]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #2801:
URL: https://github.com/apache/celeborn/pull/2801#issuecomment-2445803382
## [Codecov](https://app.codecov.io/gh/apache/celeborn/pull/2801?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
Attention: Patch coverage is `35.29412%` with `11 lines` in your changes missing coverage. Please review.
> Project coverage is 32.27%. Comparing base [(`464a7c7`)](https://app.codecov.io/gh/apache/celeborn/commit/464a7c71a932e8c85a14afc50182bb7f38c084e8?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`c6618a7`)](https://app.codecov.io/gh/apache/celeborn/commit/c6618a7672fa439da521e5417156972374dca099?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
> Report is 10 commits behind head on main.
| [Files with missing lines](https://app.codecov.io/gh/apache/celeborn/pull/2801?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...born/common/protocol/message/ControlMessages.scala](https://app.codecov.io/gh/apache/celeborn/pull/2801?src=pr&el=tree&filepath=common%2Fsrc%2Fmain%2Fscala%2Forg%2Fapache%2Fceleborn%2Fcommon%2Fprotocol%2Fmessage%2FControlMessages.scala&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29tbW9uL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvY2VsZWJvcm4vY29tbW9uL3Byb3RvY29sL21lc3NhZ2UvQ29udHJvbE1lc3NhZ2VzLnNjYWxh) | 0.00% | [10 Missing :warning: ](https://app.codecov.io/gh/apache/celeborn/pull/2801?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...cala/org/apache/celeborn/common/CelebornConf.scala](https://app.codecov.io/gh/apache/celeborn/pull/2801?src=pr&el=tree&filepath=common%2Fsrc%2Fmain%2Fscala%2Forg%2Fapache%2Fceleborn%2Fcommon%2FCelebornConf.scala&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29tbW9uL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvY2VsZWJvcm4vY29tbW9uL0NlbGVib3JuQ29uZi5zY2FsYQ==) | 85.72% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/celeborn/pull/2801?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #2801 +/- ##
==========================================
- Coverage 32.37% 32.27% -0.09%
==========================================
Files 328 329 +1
Lines 19387 19483 +96
Branches 1747 1749 +2
==========================================
+ Hits 6275 6287 +12
- Misses 12769 12853 +84
Partials 343 343
```
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/celeborn/pull/2801?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?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
Re: [PR] [CELEBORN-1577][Phase1] Storage quota should support interrupt shuffle. [celeborn]
Posted by "leixm (via GitHub)" <gi...@apache.org>.
leixm commented on code in PR #2801:
URL: https://github.com/apache/celeborn/pull/2801#discussion_r1806077486
##########
master/src/main/scala/org/apache/celeborn/service/deploy/master/Master.scala:
##########
@@ -1105,7 +1105,8 @@ private[celeborn] class Master(
(statusSystem.excludedWorkers.asScala ++ statusSystem.manuallyExcludedWorkers.asScala).asJava),
needCheckedWorkerList,
new util.ArrayList[WorkerInfo](
- (statusSystem.shutdownWorkers.asScala ++ statusSystem.decommissionWorkers.asScala).asJava)))
+ (statusSystem.shutdownWorkers.asScala ++ statusSystem.decommissionWorkers.asScala).asJava),
+ CheckQuotaResponse(isAvailable = true, "")))
Review Comment:
Currently QuotaManager only support check quota for user, at next PR, we will support check quota for application, so we don’t need to modify rpc proto at 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