SteNicholas commented on code in PR #3754:
URL: https://github.com/apache/celeborn/pull/3754#discussion_r3541498744
##########
common/src/main/scala/org/apache/celeborn/common/meta/WorkerInfo.scala:
##########
@@ -269,10 +269,12 @@ class WorkerInfo(
val userResourceConsumptionString =
if (userResourceConsumption == null || userResourceConsumption.isEmpty) {
"empty"
- } else if (userResourceConsumption != null) {
- userResourceConsumption.asScala.map { case (userIdentifier,
resourceConsumption) =>
- s"\n UserIdentifier: ${userIdentifier}, ResourceConsumption:
${resourceConsumption}"
+ } else {
+ val rendered = userResourceConsumption.asScala.iterator.collect {
+ case (userIdentifier, resourceConsumption) if
!resourceConsumption.isEmpty =>
Review Comment:
Test coverage: the PR says "Covered by existing unit tests", but the
existing `WorkerInfoSuite` "toString output" case doesn't actually exercise
this new branch. `worker4` renders a single non-zero entry, and
`worker1`-`worker3` have empty maps that hit the outer
`userResourceConsumption.isEmpty` branch. Nothing covers:
- a zero entry (`ResourceConsumption(0, 0, 0, 0)`) being filtered out,
- a mix of zero + non-zero entries (only the non-zero one rendered),
- the `if (rendered.isEmpty) "empty"` fallback when every entry is filtered.
Worth adding a small case so a future regression that re-renders zero
entries is caught.
##########
common/src/main/scala/org/apache/celeborn/common/quota/ResourceConsumption.scala:
##########
@@ -76,6 +76,11 @@ case class ResourceConsumption(
(add(other._1), addSubResourceConsumptions(other._2))
}
+ def isEmpty: Boolean = {
Review Comment:
Consistency / reuse: there's already an equivalent "filter out empty user
resource consumption" predicate that feeds `GET /api/v1/workers`.
`ApiUtils.workerResourceConsumptions` keeps an entry only when
`CollectionUtils.isNotEmpty(ur._2.subResourceConsumptions)`
(`service/src/main/scala/.../http/api/v1/ApiUtils.scala:69-70`, comment: `//
filter out user resource consumption with empty sub resource consumptions`).
This new `isEmpty` uses a different rule — the four counters **and** the
sub-map. For the shapes the worker snapshot actually produces the two agree
(active users always carry a non-empty sub-map via
`StorageManager.userResourceConsumptionSnapshot`; the zeroed placeholders
inserted by `WorkerInfo.updateThenGetUserResourceConsumption` are all-zero with
an empty sub-map). But they diverge for an entry with non-zero counters and an
empty/null sub-map, and they are now two independent definitions of the same
concept maintained separately.
Consider unifying: either have `ApiUtils` reuse this new `isEmpty`, or
filter `toString` on the same `isNotEmpty(subResourceConsumptions)` predicate,
so the text/log output and the JSON API stay in lockstep.
--
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]
