Skip to content
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

Merged
merged 1 commit into from
Feb 22, 2023

Conversation

shoothzj
Copy link
Member

@shoothzj shoothzj commented Feb 11, 2023

see #2679 for more details

@volcano-sh-bot volcano-sh-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Feb 11, 2023
@wangyang0616
Copy link
Member

wangyang0616 commented Feb 13, 2023

@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.

@wangyang0616
Copy link
Member

When the architecture is updated, it is best to update the design documents synchronously usage-based-scheduling.md

@shoothzj
Copy link
Member Author

@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.
WDYT? thanks for advance.

@shoothzj shoothzj force-pushed the extract-metrics-client branch 4 times, most recently from a5850fe to 6cf4758 Compare February 13, 2023 10:01
@wangyang0616
Copy link
Member

@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. WDYT? thanks for advance.

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 refermetrics, the metrics obtained from other platforms can also be placed in this folder uniformly.

@shoothzj shoothzj force-pushed the extract-metrics-client branch 2 times, most recently from 4130e3d to 9a6845a Compare February 14, 2023 03:18
@shoothzj
Copy link
Member Author

@wangyang0616 Hi, if no other concern, can we merge this?

@wangyang0616
Copy link
Member

It's ok in my opinion.
cc @william-wang @Thor-wl

@wangyang0616
Copy link
Member

/lgtm

@volcano-sh-bot volcano-sh-bot added the lgtm Indicates that a PR is ready to be merged. label Feb 14, 2023
@Thor-wl
Copy link
Contributor

Thor-wl commented Feb 14, 2023

It's ok in my opinion.
cc @william-wang @Thor-wl

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.

@william-wang
Copy link
Member

@shoothzj Thanks for your contribution. I will take a look as soon as possible:)

@volcano-sh-bot volcano-sh-bot removed the lgtm Indicates that a PR is ready to be merged. label Feb 20, 2023
@shoothzj shoothzj force-pushed the extract-metrics-client branch 2 times, most recently from fd6e279 to 7ef5cab Compare February 20, 2023 03:20
@volcano-sh-bot volcano-sh-bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 20, 2023
@volcano-sh-bot volcano-sh-bot removed the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Feb 20, 2023
@volcano-sh-bot volcano-sh-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Feb 20, 2023
@shoothzj shoothzj force-pushed the extract-metrics-client branch 4 times, most recently from 475c029 to 64dd65a Compare February 20, 2023 09:32
@shoothzj shoothzj force-pushed the extract-metrics-client branch 11 times, most recently from 1456183 to 9d5a50e Compare February 21, 2023 23:35
Signed-off-by: ZhangJian He <shoothzj@gmail.com>
Copy link
Member

@william-wang william-wang left a comment

Choose a reason for hiding this comment

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

/lgtm

@volcano-sh-bot volcano-sh-bot added the lgtm Indicates that a PR is ready to be merged. label Feb 22, 2023
@volcano-sh-bot
Copy link
Contributor

[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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@volcano-sh-bot volcano-sh-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 22, 2023
@jiangkaihua
Copy link
Contributor

/lgtm

@volcano-sh-bot volcano-sh-bot merged commit db82b3e into volcano-sh:master Feb 22, 2023
@shoothzj shoothzj deleted the extract-metrics-client branch February 23, 2023 02:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants