Skip to content

[GCS]Fix bug that gcs client does not set last_resource_usage_#13856

Merged
ffbin merged 3 commits intoray-project:masterfrom
antgroup:fix_last_resource_usage_bug
Feb 5, 2021
Merged

[GCS]Fix bug that gcs client does not set last_resource_usage_#13856
ffbin merged 3 commits intoray-project:masterfrom
antgroup:fix_last_resource_usage_bug

Conversation

@ffbin
Copy link
Copy Markdown
Contributor

@ffbin ffbin commented Feb 2, 2021

Why are these changes needed?

gcs client does not set last_resource_usage_ when report resource usage, so we get last_resource_usage_ is empty.

Related issue number

Checks

  • I've run scripts/format.sh to lint the changes in this PR.
  • I've included any doc changes needed for https://docs.ray.io/en/master/.
  • I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
  • Testing Strategy
    • Unit tests
    • Release tests
    • This PR is not tested :(

protected:
NodeResourceInfoAccessor() = default;

private:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We’d better to keep this private flag. There's a method called GetLastResourceUsage for accessing this var.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I made the following changes: GetLastResourceUsage returns the const pointer to prevent external modification of the value of last_resource_usage_, thx.

Copy link
Copy Markdown
Contributor

@WangTaoTheTonic WangTaoTheTonic left a comment

Choose a reason for hiding this comment

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

Looks like setting resource usage cache was missed in new scheduling. Nice catch!
Just a tiny comment!

@rkooo567
Copy link
Copy Markdown
Contributor

rkooo567 commented Feb 3, 2021

One question. What was the consequence of not having this before? I wonder why we couldn't catch this. How serious the issue was?

Copy link
Copy Markdown
Contributor

@rkooo567 rkooo567 left a comment

Choose a reason for hiding this comment

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

Please answer the question above!

@rkooo567 rkooo567 added the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Feb 4, 2021
@ffbin
Copy link
Copy Markdown
Contributor Author

ffbin commented Feb 4, 2021

One question. What was the consequence of not having this before? I wonder why we couldn't catch this. How serious the issue was?

When new scheduler is enabled, it will affect the lightweight heartbeat reporting, because it does not set the node resource reported last time, so it is always considered that the node resource has changed and the node resource information needs to be reported. It affects performance, not functionality, so it's not found.

@ffbin ffbin merged commit 8a5999c into ray-project:master Feb 5, 2021
@WangTaoTheTonic WangTaoTheTonic deleted the fix_last_resource_usage_bug branch February 5, 2021 04:03
fishbone added a commit to fishbone/ray that referenced this pull request Feb 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

@author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants