Copilot commented on code in PR #3739:
URL: https://github.com/apache/celeborn/pull/3739#discussion_r3413194281
##########
docs/configuration/master.md:
##########
@@ -37,6 +37,8 @@ license: |
| celeborn.internal.port.enabled | false | false | Whether to create a
internal port on Masters/Workers for inter-Masters/Workers communication. This
is beneficial when SASL authentication is enforced for all interactions between
clients and Celeborn Services, but the services can exchange messages without
being subject to SASL authentication. | 0.5.0 | |
| celeborn.logConf.enabled | false | false | When `true`, log the CelebornConf
for debugging purposes. | 0.5.0 | |
| celeborn.master.allowWorkerHostPattern | <undefined> | false | Pattern
of worker host that allowed to register with the master. If not set, all
workers are allowed to register. | 0.6.0 | |
+| celeborn.master.clusterOverload.gc.enabled | false | false | Whether to
enable the master signaling clients to trigger GC when the cluster storage is
overloaded (disk usage exceeds the threshold). | 0.7.0 | |
+| celeborn.master.clusterOverload.gc.threshold | 0.9 | false | Fraction of
total cluster disk capacity used (0.0–1.0) above which the master signals
clients to trigger GC to release stale shuffle dependencies. For example, 0.9
means 90% of total capacity is in use. | 0.7.0 | |
Review Comment:
The documented threshold range and comparison semantics don’t match the
implementation. `MASTER_CLUSTER_OVERLOAD_GC_THRESHOLD` is validated as (0, 1]
and `Master.isClusterOverloaded` triggers when usedFraction >= threshold
(inclusive), but this row documents 0.0–1.0 and “above which” (strict). Please
align docs with the code to avoid confusing operators and users.
##########
common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala:
##########
@@ -2517,6 +2522,26 @@ object CelebornConf extends Logging {
.timeConf(TimeUnit.MILLISECONDS)
.createWithDefaultString("300s")
+ val MASTER_CLUSTER_OVERLOAD_GC_ENABLED: ConfigEntry[Boolean] =
+ buildConf("celeborn.master.clusterOverload.gc.enabled")
+ .categories("master")
+ .version("0.7.0")
+ .doc("Whether to enable the master signaling clients to trigger GC when
the cluster " +
+ "storage is overloaded (disk usage exceeds the threshold).")
+ .booleanConf
+ .createWithDefault(false)
+
+ val MASTER_CLUSTER_OVERLOAD_GC_THRESHOLD: ConfigEntry[Double] =
+ buildConf("celeborn.master.clusterOverload.gc.threshold")
+ .categories("master")
+ .version("0.7.0")
+ .doc("Fraction of total cluster disk capacity used (0.0–1.0) above which
the master " +
+ "signals clients to trigger GC to release stale shuffle dependencies.
For example, " +
+ "0.9 means 90% of total capacity is in use.")
+ .doubleConf
+ .checkValue(v => v > 0.0 && v <= 1.0, "Must be between 0 (exclusive) and
1 (inclusive)")
+ .createWithDefault(0.9)
Review Comment:
The config docstring says the threshold is “(0.0–1.0) above which”, but the
value check enforces (0,1] and the overload check uses `>=` (inclusive). This
mismatch can cause operator confusion; please update the docstring to reflect
the actual valid range and inclusive comparison (or adjust
validation/comparison to match the docs).
##########
master/src/main/scala/org/apache/celeborn/service/deploy/master/Master.scala:
##########
@@ -1255,12 +1255,19 @@ private[celeborn] class Master(
new util.ArrayList[WorkerInfo](
(statusSystem.shutdownWorkers.asScala ++
statusSystem.decommissionWorkers.asScala).asJava),
new util.ArrayList(appRelatedShuffles),
- quotaManager.checkApplicationQuotaStatus(appId)))
+ quotaManager.checkApplicationQuotaStatus(appId),
+ shouldTriggerGcForApp()))
} else {
context.reply(OneWayMessageResponse)
}
}
+ private[master] def shouldTriggerGcForApp(): Boolean =
+ Master.isClusterOverloaded(
+ conf,
+ statusSystem.workersMap,
+ statusSystem.availableWorkers)
Review Comment:
`shouldTriggerGcForApp()` is invoked for every application heartbeat
response and currently recomputes cluster totals by iterating all
workers/available workers each time (`isClusterOverloaded` sums over the
collections). On busy masters with many apps heartbeating, this introduces an
O(#workers) cost per heartbeat and can become a noticeable CPU overhead under
load (the exact scenario this feature targets). Consider caching the overloaded
decision for a short TTL (e.g., 1–5s) or computing it periodically once per
master and reusing the value for all app heartbeats within that interval.
--
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]
