[resource-reporting 3/n] further clean up LocalResourceManager #21927
Conversation
scv119
force-pushed
the
remove-local-part
branch
from
40e48dd
to
26c759b
Compare
src/ray/raylet/node_manager.cc
Outdated
| @@ -587,7 +587,7 @@ void NodeManager::FillResourceReport(rpc::ResourcesData &resources_data) { | |||
| resources_data.set_node_manager_address(initial_config_.node_manager_address); | |||
| // Update local cache from gcs remote cache, this is needed when gcs restart. | |||
| // We should always keep the cache view consistent. | |||
| cluster_resource_scheduler_->GetLocalResourceManager().UpdateLastResourceUsage( | |||
| cluster_resource_scheduler_->GetLocalResourceManager().ResetResources( | |||
There was a problem hiding this comment.
I feel this name is confusing: for local resource, current node should be the source of truth. Why do we need to update local resource usage from gcs client?
There was a problem hiding this comment.
I think the better name should be ResetLastReportResourceUsage or something similar.
| void LocalResourceManager::UpdateLastResourceUsage( | ||
| std::shared_ptr<SchedulingResources> gcs_resources) { | ||
| void LocalResourceManager::ResetResources( | ||
| std::shared_ptr<SchedulingResources> replacement) { |
| gcs_resources->GetAvailableResources().GetResourceMap())); | ||
| resource_name_to_id_, replacement->GetTotalResources().GetResourceMap(), | ||
| replacement->GetAvailableResources().GetResourceMap())); | ||
| OnResourceChanged(); |
There was a problem hiding this comment.
We didn't actually change local resource, we only change the the one we report to gcs
There was a problem hiding this comment.
oh you are right!... was a bit confused
There was a problem hiding this comment.
We shouldn't need to call OnResourceChanged() here right since local resource is not changed
| /// It is responsible for allocating/deallocating resources for (task) resource request; | ||
| /// it also supports creating a new resource or delete an existing resource. | ||
| /// Whenever the resouce changes, it notifies the subscriber of the change. | ||
| /// This class is not thread safe. |
There was a problem hiding this comment.
Could we add which thread it is expected to be running in? like which event loop.
There was a problem hiding this comment.
We can revisit this once we finished the cluster resource manager part. I'm not sure if this class should be aware of the event loop.. yet.
| gcs_resources->GetAvailableResources().GetResourceMap())); | ||
| resource_name_to_id_, replacement->GetTotalResources().GetResourceMap(), | ||
| replacement->GetAvailableResources().GetResourceMap())); | ||
| OnResourceChanged(); |
There was a problem hiding this comment.
We shouldn't need to call OnResourceChanged() here right since local resource is not changed
Why are these changes needed?
Further clean up LocalResourceManager.
Related issue number
Checks
scripts/format.shto lint the changes in this PR.