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.

Sign up for GitHub

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

Jump to bottom

Resource Interpreter Framework cache and reorganize configurations #2719

Merged
merged 1 commit into from
Nov 7, 2022
Merged

Resource Interpreter Framework cache and reorganize configurations #2719

merged 1 commit into from
Nov 7, 2022

Conversation

jameszhangyukun
Copy link
Contributor

@jameszhangyukun jameszhangyukun commented Nov 1, 2022 •
edited
Loading

Signed-off-by: zhangyukun 38148677+jameszhangyukun@users.noreply.github.com

What type of PR is this?
/kind feature

What this PR does / why we need it:
add framework cache and reorganize configurations Resource Interpreter
Which issue(s) this PR fixes:
Part of #2371
Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE (Part feature)

@karmada-bot karmada-bot added kind/feature Categorizes issue or PR as related to a new feature. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Nov 1, 2022
@karmada-bot karmada-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Nov 1, 2022
@XiShanYongYe-Chang
Copy link
Member

Hi @jameszhangyukun, please fix the lint error.

@XiShanYongYe-Chang
Copy link
Member

/assign

@jameszhangyukun jameszhangyukun force-pushed the cache-framework branch 2 times, most recently from ee9fc23 to ed8c623 Compare November 1, 2022 12:16
@jameszhangyukun jameszhangyukun changed the title [WIP]Resource Interpreter Framework cache and reorganize configurations Resource Interpreter Framework cache and reorganize configurations Nov 2, 2022
@karmada-bot karmada-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 2, 2022
@jameszhangyukun
Copy link
Contributor Author

cc @XiShanYongYe-Chang

@jameszhangyukun jameszhangyukun force-pushed the cache-framework branch 4 times, most recently from 564fb4b to 0198456 Compare November 2, 2022 07:50
XiShanYongYe-Chang
XiShanYongYe-Chang reviewed Nov 2, 2022
View reviewed changes
Copy link
Member

@XiShanYongYe-Chang XiShanYongYe-Chang left a comment

Choose a reason for hiding this comment

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

Good job!

/cc @RainbowMango @chaunceyjiang Can you help take a look?

pkg/resourceinterpreter/configurableinterpreter/configurable.go Outdated Show resolved Hide resolved
pkg/resourceinterpreter/configurableinterpreter/configurable.go Show resolved Hide resolved
pkg/resourceinterpreter/configurableinterpreter/configurable.go Outdated Show resolved Hide resolved
pkg/resourceinterpreter/configurableinterpreter/configurable.go Outdated Show resolved Hide resolved
@ikaven1024
Copy link
Member

ikaven1024 commented Nov 2, 2022 •
edited
Loading

Based this pr, I have Refactored something. You can see and compare it. @jameszhangyukun @XiShanYongYe-Chang

master...ikaven1024:karmada:cache-framework

@jameszhangyukun
Copy link
Contributor Author

jameszhangyukun commented Nov 3, 2022 •
edited
Loading

Based this pr, I have Refactored something. You can see and compare it. @jameszhangyukun @XiShanYongYe-Chang

master...ikaven1024:karmada:cache-framework

Good Job! thanks. Can i use this code in my pr ?

@ikaven1024
Copy link
Member

Good Job! thanks. Can i use this code in my pr ?

This framework is not finished. I will continue this work based on you PR. And your contribution will also counted.

You can do the implement of Lua runtime, it's also a hard work.

@XiShanYongYe-Chang
Copy link
Member

This framework is not finished. I will continue this work based on you PR. And your contribution will also counted.

You can do the implement of Lua runtime, it's also a hard work.

I'd like to speed up the process, and we can show that at the meeting next Tuesday.

@ikaven1024
Copy link
Member

I'd like to speed up the process, and we can show that at the meeting next Tuesday.

OK, I will finish it today.

@XiShanYongYe-Chang
Copy link
Member

I'd like to speed up the process, and we can show that at the meeting next Tuesday.

OK, I will finish it today.

Thanks a lot~

@XiShanYongYe-Chang XiShanYongYe-Chang mentioned this pull request Nov 3, 2022
Closed
@RainbowMango
Copy link
Member

I just went through the code quickly, and it generally looks good.
I do find some nits, and will post them later.
I want to know what's the current situation.

@ikaven1024 I see you also send a PR for this, why?

@ikaven1024
Copy link
Member

@ikaven1024 I see you also send a PR for this, why?

My pr is based on this pr, and improving:

  • Script will be compiled only when config are added or updated, and the compiled function are cached. I will save some time.
  • Easy to extend other script。 ONLY implement Load and Invoke methods. No need to implement every actual caller (such as GetReplicas).

@jameszhangyukun also feels it' OK.

@RainbowMango
Copy link
Member

My pr is based on this pr, and improving:

Yeah, I see, thanks for that you kept the @jameszhangyukun's commit in #2733.

Given the task is part of the LFX program, the contribution should be recorded and posted on @jameszhangyukun's evaluation.
Can we move this PR forward and do the rest improvement at #2733?

@ikaven1024
Copy link
Member

Can we move this PR forward and do the rest improvement at #2733?

OK. @jameszhangyukun you can continue your PR. I will do the improvement later.

RainbowMango
RainbowMango reviewed Nov 4, 2022
View reviewed changes
pkg/resourceinterpreter/configurableinterpreter/configurable.go Outdated Show resolved Hide resolved
pkg/resourceinterpreter/configurableinterpreter/configurable.go Outdated Show resolved Hide resolved
pkg/resourceinterpreter/configurableinterpreter/configurable.go Outdated Show resolved Hide resolved
pkg/resourceinterpreter/configurableinterpreter/configurable.go Outdated Show resolved Hide resolved
pkg/resourceinterpreter/configurableinterpreter/configmanager/manager.go Outdated Show resolved Hide resolved
pkg/resourceinterpreter/configurableinterpreter/configmanager/manager.go Outdated Show resolved Hide resolved
pkg/resourceinterpreter/configurableinterpreter/configmanager/manager.go Outdated Show resolved Hide resolved
pkg/resourceinterpreter/configurableinterpreter/configmanager/manager.go Outdated Show resolved Hide resolved
pkg/resourceinterpreter/configurableinterpreter/configmanager/manager.go Outdated Show resolved Hide resolved
pkg/resourceinterpreter/configurableinterpreter/configmanager/accessor.go Outdated Show resolved Hide resolved
@RainbowMango
Copy link
Member

Please @jameszhangyukun fix the comments above and I'll take a second round review. Thanks.

@jameszhangyukun
Copy link
Contributor Author

cc @RainbowMango @XiShanYongYe-Chang

XiShanYongYe-Chang
XiShanYongYe-Chang reviewed Nov 7, 2022
View reviewed changes
Copy link
Member

@XiShanYongYe-Chang XiShanYongYe-Chang left a comment

Choose a reason for hiding this comment

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

/lgtm

@karmada-bot karmada-bot added the lgtm Indicates that a PR is ready to be merged. label Nov 7, 2022
@RainbowMango RainbowMango mentioned this pull request Nov 7, 2022
Closed
8 tasks
RainbowMango
RainbowMango reviewed Nov 7, 2022
View reviewed changes
Copy link
Member

@RainbowMango RainbowMango left a comment

Choose a reason for hiding this comment

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

Last nit from me.

pkg/resourceinterpreter/configurableinterpreter/configurable.go Outdated Show resolved Hide resolved
@jameszhangyukun
Resource Interpreter Framework cache and reorganize configurations
a3c50e7 
Signed-off-by: zhangyukun <38148677+jameszhangyukun@users.noreply.github.com>
@karmada-bot karmada-bot removed the lgtm Indicates that a PR is ready to be merged. label Nov 7, 2022
RainbowMango
RainbowMango approved these changes Nov 7, 2022
View reviewed changes
Copy link
Member

@RainbowMango RainbowMango left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@karmada-bot karmada-bot added the lgtm Indicates that a PR is ready to be merged. label Nov 7, 2022
@karmada-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: RainbowMango

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

@karmada-bot karmada-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 7, 2022
@karmada-bot karmada-bot merged commit c622e23 into karmada-io:master Nov 7, 2022
@RainbowMango RainbowMango mentioned this pull request Nov 15, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ikaven1024 ikaven1024 ikaven1024 left review comments

@chaunceyjiang chaunceyjiang chaunceyjiang left review comments

@XiShanYongYe-Chang XiShanYongYe-Chang XiShanYongYe-Chang left review comments

@RainbowMango RainbowMango RainbowMango approved these changes

Assignees

@RainbowMango RainbowMango

@XiShanYongYe-Chang XiShanYongYe-Chang

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/feature Categorizes issue or PR as related to a new feature. 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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@jameszhangyukun @XiShanYongYe-Chang @ikaven1024 @RainbowMango @karmada-bot @chaunceyjiang