Copilot commented on code in PR #3739:
URL: https://github.com/apache/celeborn/pull/3739#discussion_r3593221564
##########
master/src/main/scala/org/apache/celeborn/service/deploy/master/quota/QuotaManager.scala:
##########
@@ -141,6 +153,28 @@ class QuotaManager(
checkQuotaSpace(CLUSTER_EXHAUSTED, consumption, getClusterStorageQuota)
}
+ // Calling the cluster overloaded (leads to gc triggers on app side to
relieve storage)
+ private def checkClusterOverloaded(consumption: ResourceConsumption):
Boolean = {
+ val overloadQuota = getClusterStorageQuota
+ val factor = getClusterOverloadLimitFactor
+ def scale(q: Long): Long = {
+ if (q <= 0L || q == Long.MaxValue) q
+ else math.max(1L, math.ceil(factor * q.toDouble).toLong)
+ }
Review Comment:
`scale()` uses `math.ceil(factor * q)` which rounds the overload threshold
*up*. For small quotas (e.g., file-count limits) this can effectively make the
overload threshold equal to the full quota, contradicting the documented "quota
* factor" behavior and making overload GC much less likely to trigger than
intended. Using `toLong`/`floor` keeps the threshold at or below `quota *
factor` and avoids occasional off-by-one due to floating-point rounding +
`ceil`.
##########
docs/configuration/quota.md:
##########
@@ -26,6 +26,7 @@ license: |
| celeborn.quota.cluster.hdfsFileCount | 9223372036854775807 | true | Cluster
level quota dynamic configuration for written hdfs file count. | 0.6.0 | |
| celeborn.quota.enabled | true | false | When Master side sets to true, the
master will enable to check the quota via QuotaManager. When Client side sets
to true, LifecycleManager will request Master side to check whether the current
user has enough quota before registration of shuffle. Fallback to the default
shuffle service when Master side checks that there is no enough quota for
current user. | 0.2.0 | |
| celeborn.quota.interruptShuffle.enabled | false | false | Whether to enable
interrupt shuffle when quota exceeds. | 0.6.0 | |
+| celeborn.quota.overload.factor | 0.8 | true | This config decides the quota
* factor at which to consider the cluster 'overloaded'. When the cluster is
overloaded, application heartbeat responses contain a signal to trigger a GC to
clean up dangling shuffle dependencies | 0.7.0 | |
Review Comment:
The description for `celeborn.quota.overload.factor` implies the GC signal
is always sent when the cluster is overloaded, but in code it is additionally
gated by `celeborn.master.clusterOverload.gc.enabled`. Documenting that guard
here would prevent confusion for operators changing the factor and expecting
behavior with the master flag still disabled.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
