Copilot commented on code in PR #3739:
URL: https://github.com/apache/celeborn/pull/3739#discussion_r3592980862
##########
client/src/main/scala/org/apache/celeborn/client/ApplicationHeartbeater.scala:
##########
@@ -47,6 +47,10 @@ class ApplicationHeartbeater(
private var stopped = false
private val reviseLostShuffles = conf.reviseLostShufflesEnabled
+ private val gcOnOverloadEnabled = conf.clientGcOnOverloadEnabled
+ private val gcOnOverloadMinIntervalMs = conf.clientGcOnOverloadMinIntervalMs
+ @volatile private[client] var lastGcTriggerTimeMs = 0L
+
Review Comment:
Cooldown timing uses wall-clock time (`System.currentTimeMillis`) but the GC
throttling decision should be based on a monotonic clock to avoid misbehaving
when the system clock jumps (e.g., NTP adjustments). Consider adding a
`lastGcTriggerTimeNs` (and precomputed `gcOnOverloadMinIntervalNs`) for the
interval comparison, while keeping `lastGcTriggerTimeMs` only for
observability/tests.
##########
common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala:
##########
@@ -4776,6 +4789,25 @@ object CelebornConf extends Logging {
.booleanConf
.createWithDefault(true)
+ val CLIENT_GC_ON_CLUSTER_OVERLOAD_ENABLED: ConfigEntry[Boolean] =
+ buildConf("celeborn.client.clusterOverload.gc.enabled")
+ .categories("client")
+ .version("0.7.0")
+ .doc("When true, the client will trigger System.gc() upon receiving a GC
signal from " +
+ "the master indicating cluster storage is overloaded. Disable to
ignore the signal.")
+ .booleanConf
+ .createWithDefault(true)
+
+ val CLIENT_GC_ON_CLUSTER_OVERLOAD_MIN_INTERVAL: ConfigEntry[Long] =
+ buildConf("celeborn.client.clusterOverload.gc.minInterval")
+ .categories("client")
+ .version("0.7.0")
+ .doc("Minimum time that must elapse between consecutive GC triggers on
the client side " +
+ "in response to master GC signals. This prevents excessive GC pressure
when the " +
+ "cluster remains overloaded across multiple heartbeat intervals.")
+ .timeConf(TimeUnit.MILLISECONDS)
+ .createWithDefaultString("5m")
Review Comment:
`celeborn.client.clusterOverload.gc.minInterval` currently accepts negative
values, which would effectively disable throttling (because `now - last >=
negative` is always true). Add a config validation to enforce a non-negative
interval.
##########
common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala:
##########
@@ -6895,6 +6927,18 @@ 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
+ .checkValue(v => v >= 0.0 && v <= 1.0, "Should be in [0.0, 1.0].")
+ .createWithDefault(0.8)
Review Comment:
`celeborn.quota.overload.factor` is documented as `quota * factor`, and
validation currently allows `0.0`, but `QuotaManager.checkClusterOverloaded`
clamps thresholds to at least 1 for any positive quota. This makes `factor=0.0`
behave unexpectedly (threshold becomes 1, not 0). Either disallow 0.0 here
(recommended to avoid accidental always-on signaling semantics) or adjust the
overload threshold computation to match the documented formula.
##########
client/src/main/scala/org/apache/celeborn/client/ApplicationHeartbeater.scala:
##########
@@ -170,6 +175,21 @@ class ApplicationHeartbeater(
}
}
+ private[client] def handleGcSignal(shouldTriggerGc: Boolean): Unit = {
+ if (!gcOnOverloadEnabled || !shouldTriggerGc) return
+ val now = System.currentTimeMillis()
+ if (now - lastGcTriggerTimeMs >= gcOnOverloadMinIntervalMs) {
+ logInfo(
+ "Cluster is overloaded; triggering System.gc() to release stale
shuffle dependencies.")
+ lastGcTriggerTimeMs = now
+ System.gc()
+ } else {
+ logDebug(
+ s"Cluster overload GC signal received but skipped: last GC was
triggered " +
+ s"${now - lastGcTriggerTimeMs}ms ago (min interval:
${gcOnOverloadMinIntervalMs}ms).")
+ }
+ }
Review Comment:
GC throttle uses `System.currentTimeMillis()` to measure elapsed time, which
can move backwards/forwards with wall-clock adjustments and can cause GC to be
skipped or triggered too frequently. Use `System.nanoTime()` (monotonic) for
the cooldown check, and only use `currentTimeMillis()` for recording/logging if
needed.
--
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]
