[CELEBORN-1577][Phase2] QuotaManager should support interrupt shuffle. #2819
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
RexXiong
reviewed
Nov 4, 2024
| @@ -671,8 +671,14 @@ private[celeborn] class Worker( | |||
| val resourceConsumptionSnapshot = storageManager.userResourceConsumptionSnapshot() | |||
| val userResourceConsumptions = | |||
| workerInfo.updateThenGetUserResourceConsumption(resourceConsumptionSnapshot.asJava) | |||
| resourceConsumptionSnapshot.foreach { case (userIdentifier, _) => | |||
| resourceConsumptionSnapshot.foreach { case (userIdentifier, userResourceConsumption) => | |||
There was a problem hiding this comment.
It would be better not persist resourceConsumption into ratis When worker heartbeat subResourceConsumption
| masterSource.sample(UPDATE_RESOURCE_CONSUMPTION_TIME, this.getClass.getSimpleName, Map.empty) { | ||
| val clusterQuota = getClusterStorageQuota | ||
| var clusterResourceConsumption = ResourceConsumption(0, 0, 0, 0) | ||
| val userResourceConsumption = statusSystem.workerSnapshot.asScala.flatMap { workerInfo => |
There was a problem hiding this comment.
Lack of tenant-level quota checks.
| @@ -504,4 +501,10 @@ public boolean isWorkerAvailable(WorkerInfo workerInfo) { | |||
| public void updateApplicationMeta(ApplicationMeta applicationMeta) { | |||
| applicationMetas.putIfAbsent(applicationMeta.appId(), applicationMeta); | |||
| } | |||
|
|
|||
| public List<WorkerInfo> workerSnapshot() { | |||
There was a problem hiding this comment.
Why not directly use workers for computing resource consumption?
| quotaChecker.scheduleWithFixedDelay( | ||
| () => { | ||
| try { | ||
| updateResourceConsumption() |
There was a problem hiding this comment.
this method not ly update source consumption but also check quota for user-level, app-level, so may use another name or separate the logic
|
This PR is stale because it has been open 20 days with no activity. Remove stale label or comment or this will be closed in 10 days. |
leixm
force-pushed
the
CELEBORN-1577-2
branch
from
ff37e63 to
940609e
Compare
|
I will fix above problems later. |
leixm
force-pushed
the
CELEBORN-1577-2
branch
from
a13e20b to
1b94be6
Compare
leixm
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
2.1 When the tenant's resourceConsumption exceeds the tenant's quota, select the app with a larger consumption to mark interrupted.
2.2 When the resourceConsumption of the cluster exceeds the cluster quota, select the app with larger consumption to mark interrupted.
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?
UTs.