Skip to content

[CELEBORN-1758] Remove the empty user resource consumption from worker heartbeat#2967

Closed
turboFei wants to merge 1 commit intoapache:mainfrom
turboFei:reduce_report
Closed

[CELEBORN-1758] Remove the empty user resource consumption from worker heartbeat#2967
turboFei wants to merge 1 commit intoapache:mainfrom
turboFei:reduce_report

Conversation

@turboFei
Copy link
Copy Markdown
Member

@turboFei turboFei commented Dec 2, 2024

What changes were proposed in this pull request?

  1. report the resourceConsumptionSnapshot from storageManager directly in the worker heartbeat, which does not contain the empty user resource consumption
  2. For RESTful API, do not return the empty user resource consumption as well.

Why are the changes needed?

def updateThenGetUserResourceConsumption(resourceConsumptions: util.Map[
UserIdentifier,
ResourceConsumption]): util.Map[UserIdentifier, ResourceConsumption] = {
userResourceConsumption.keys().asScala.filterNot(resourceConsumptions.containsKey).foreach {
identifier =>
userResourceConsumption.put(identifier, ResourceConsumption(0, 0, 0, 0))
}
userResourceConsumption.putAll(resourceConsumptions)
userResourceConsumption
}

Currently, we never remove the user resource consumption even the sub resource consumptions is empty, and create a ResourceConsumption(0, 0, 0, 0) instead.

I am afraid that, the worker will report more and more empty user resource consumption to master, once one of their slots assigned to this worker.

Likes:
image

So, I think we just need to report the resourceConsumptionSnapshot from storageManager directly, which does not contain the empty user resource consumption.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

GA.

Copy link
Copy Markdown
Contributor

@FMX FMX left a comment

Choose a reason for hiding this comment

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

LGTM. After checking all related codes, this PR won't introduce trouble. But I'll leave this PR for others to merge.

Copy link
Copy Markdown
Member

@SteNicholas SteNicholas left a comment

Choose a reason for hiding this comment

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

LGTM.

@SteNicholas
Copy link
Copy Markdown
Member

Merged to main(v0.6.0).

@turboFei turboFei deleted the reduce_report branch December 6, 2024 01:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants