-
Notifications
You must be signed in to change notification settings - Fork 953
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.
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
Extract MetricsClient and NodeMetrics to support other metrics platform #2678
Extract MetricsClient and NodeMetrics to support other metrics platform #2678
Conversation
21f1448
to
b9e7632
Compare
@shoothzj Thank you for your contribution. It will be more beneficial for everyone to review if you can add a PR description, such as the background, reason and goal of this PR, etc. If there is a corresponding issue, you can also directly associate it. |
When the architecture is updated, it is best to update the design documents synchronously usage-based-scheduling.md |
b9e7632
to
acc31e3
Compare
@wangyang0616 I have already created a ticket (#2679) to track this enhancement. I suggest that we don't update the document in this pull request. Instead, I prefer to update the document when step 2 is implemented, which involves the introduction of the "metrics.type" variable. |
a5850fe
to
6cf4758
Compare
This is very good. By the way, is it better to add a folder under the cache directory to store the newly added metrics files, such as |
4130e3d
to
9a6845a
Compare
@wangyang0616 Hi, if no other concern, can we merge this? |
It's ok in my opinion. |
/lgtm |
I'm sorry for the late reply. Since I'm a little bit busy recently, will reivew recent PRs and issues together sometime this weekend. |
@shoothzj Thanks for your contribution. I will take a look as soon as possible:) |
9a6845a
to
15d8be7
Compare
fd6e279
to
7ef5cab
Compare
7ef5cab
to
683f472
Compare
475c029
to
64dd65a
Compare
1456183
to
9d5a50e
Compare
Signed-off-by: ZhangJian He <shoothzj@gmail.com>
9d5a50e
to
c93066f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: william-wang The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/lgtm |
see #2679 for more details