Copilot commented on code in PR #3739:
URL: https://github.com/apache/celeborn/pull/3739#discussion_r3587869449
##########
master/src/main/scala/org/apache/celeborn/service/deploy/master/quota/QuotaManager.scala:
##########
@@ -141,6 +153,20 @@ 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 clusterOverloadFactor = getClusterOverloadLimitFactor
+ checkQuotaSpace(
+ "cluster overloaded",
+ consumption,
+ new StorageQuota(
+ diskBytesWritten = (clusterOverloadFactor *
overloadQuota.diskBytesWritten).toLong,
+ diskFileCount = (clusterOverloadFactor *
overloadQuota.diskFileCount).toLong,
+ hdfsBytesWritten = (clusterOverloadFactor *
overloadQuota.hdfsBytesWritten).toLong,
+ hdfsFileCount = (clusterOverloadFactor *
overloadQuota.hdfsFileCount).toLong)).exceed
+ }
Review Comment:
`checkClusterOverloaded` currently reuses `checkQuotaSpace`, which logs WARN
messages when thresholds are exceeded and truncates `factor * quota` with
`.toLong` (can round small quotas down to 0, effectively disabling checks
because `checkQuota` ignores `quota <= 0`). Consider computing the scaled
threshold with `ceil`/min-1 and using `checkConsumptionExceeded` to avoid log
spam while the cluster remains overloaded.
##########
common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala:
##########
@@ -6895,6 +6927,17 @@ object CelebornConf extends Logging {
.checkValue(v => v >= 0.0 && v < 1.0, "Should be in [0.0, 1).")
.createWithDefault(0.3)
+ val QUOTA_CLUSTER_OVERLOAD_FACTOR: ConfigEntry[Double] =
+ buildConf("celeborn.quota.overload.factor")
+ .categories("quota")
+ .dynamic
+ .doc("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")
+ .version("0.7.0")
+ .doubleConf
+ .createWithDefault(0.8)
Review Comment:
`celeborn.quota.overload.factor` is used as a multiplier for overload
thresholds but currently has no value validation, so negative values (or values
far outside the expected range) can silently disable/warp overload detection.
Adding a `checkValue` (and fixing the unmatched quote in the doc string) would
make misconfiguration fail fast.
##########
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` has an unmatched quote
(`cluster 'overloaded.`) and inconsistent capitalization for GC, which makes
the rendered config docs confusing.
--
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]
