Skip to content

[CELEBORN-2376] Filter empty ResourceConsumption entries in WorkerInfo toString output #3754

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
gaoyajun02 wants to merge 2 commits into
base: main
Choose a base branch
from
+10 −3
Open

[CELEBORN-2376] Filter empty ResourceConsumption entries in WorkerInfo toString output #3754

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9e64b0d
[MINOR] Filter empty ResourceConsumption entries in WorkerInfo toStri…
Jun 29, 2026
a9a57b9
Potential fix for pull request finding
gaoyajun02 Jul 7, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions common/src/main/scala/org/apache/celeborn/common/meta/WorkerInfo.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 =>

@SteNicholas SteNicholas Jul 8, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

s"\n UserIdentifier: ${userIdentifier}, ResourceConsumption: ${resourceConsumption}"
}.mkString("")
if (rendered.isEmpty) "empty" else rendered
Comment on lines +273 to +277
}
s"""
|Host: $host
Expand Down
5 changes: 5 additions & 0 deletions common/src/main/scala/org/apache/celeborn/common/quota/ResourceConsumption.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,11 @@ case class ResourceConsumption(
(add(other._1), addSubResourceConsumptions(other._2))
}

def isEmpty: Boolean = {

@SteNicholas SteNicholas Jul 8, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

diskBytesWritten == 0 && diskFileCount == 0 && hdfsBytesWritten == 0 && hdfsFileCount == 0 &&
CollectionUtils.isEmpty(subResourceConsumptions)
}

override def toString: String = {
val subResourceConsumptionString =
if (CollectionUtils.isEmpty(subResourceConsumptions)) {
Expand Down
Loading