Copilot commented on code in PR #3754:
URL: https://github.com/apache/celeborn/pull/3754#discussion_r3526552449
##########
common/src/main/scala/org/apache/celeborn/common/meta/WorkerInfo.scala:
##########
@@ -269,10 +269,15 @@ 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}"
- }.mkString("")
+ } else {
+ val nonEmpty = userResourceConsumption.asScala.filterNot(_._2.isEmpty)
+ if (nonEmpty.isEmpty) {
+ "empty"
Review Comment:
The new behavior (filtering out users whose `ResourceConsumption` is
entirely zero, and returning "empty" when everything is filtered) doesn’t
appear to be covered by the existing `WorkerInfoSuite` `toString` assertions,
which only exercise null/empty maps and a non-zero entry. Adding a unit test
case for a map containing only `ResourceConsumption(0,0,0,0)` (and a mixed map)
would prevent regressions in this user-facing output.
##########
common/src/main/scala/org/apache/celeborn/common/meta/WorkerInfo.scala:
##########
@@ -269,10 +269,15 @@ 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}"
- }.mkString("")
+ } else {
+ val nonEmpty = userResourceConsumption.asScala.filterNot(_._2.isEmpty)
+ if (nonEmpty.isEmpty) {
+ "empty"
+ } else {
+ nonEmpty.map { case (userIdentifier, resourceConsumption) =>
+ s"\n UserIdentifier: ${userIdentifier}, ResourceConsumption:
${resourceConsumption}"
+ }.mkString("")
+ }
Review Comment:
`toString` builds an intermediate filtered Map (`filterNot`) and then
iterates again to render the output. This adds avoidable allocation and extra
traversal for large `userResourceConsumption` maps; you can render in a single
pass and decide between "empty" vs non-empty based on the rendered string.
--
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]
