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

proposal - Sedna controller enhancement #437

Merged

Conversation

SherlockShemol
Copy link
Contributor

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #

Submit a first draft of proposal of sedna controller enhancement

Signed-off-by: shemol <[email protected]>
…r reference

correct the owner reference of joint inference objects

Signed-off-by: shemol <[email protected]>
@kubeedge-bot kubeedge-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 7, 2024
@SherlockShemol SherlockShemol changed the title Sedna controller enhancement proposal - Sedna controller enhancement Aug 7, 2024
Copy link

@MooreZheng MooreZheng left a comment

Choose a reason for hiding this comment

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

This PR fixed #430
Previous comments can be found at #435

The concerns about compatibility are considered in this proposal. As mentioned in the scope, "Sedna joint inference and federated learning controller optimization only modifies the joint inference and federated learning controller codes. The controllers are decoupled from each other, and the function interface and library functions are not modified, so it will not affect the functions of other controllers. For existing application cases, since the CRD is not modified, the deployment of existing applications will not be affected."

The proposal looks good to me. We also got another lgtm at the routine meeting. It would be fine to merge now. However, the bug fix is not a trivial job. Sedna is also quite a complicated system. The proposal could be developed in a continuous manner and be modified during future implementations if needed.

@kubeedge-bot kubeedge-bot added the lgtm Indicates that a PR is ready to be merged. label Aug 13, 2024
@kubeedge-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: MooreZheng

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

@kubeedge-bot kubeedge-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 13, 2024
@kubeedge-bot kubeedge-bot merged commit 2ecc30d into kubeedge:main Aug 13, 2024
2 checks passed
@kubeedge-bot
Copy link
Collaborator

@MooreZheng: Failed to re-open PR: state cannot be changed. The pull request cannot be reopened.

In response to this:

/reopen

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@MooreZheng
Copy link

MooreZheng commented Aug 13, 2024

The proposal could be developed in a continuous manner and be modified during future implementations if needed.

Comment on lines -65 to 66
var Kind = sednav1.SchemeGroupVersion.WithKind(Name)
var Kind = sednav1.SchemeGroupVersion.WithKind(KindName)

Copy link
Contributor

Choose a reason for hiding this comment

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

The bug fix and documentation PRs should be submitted separately. Please keep this in mind for future submissions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I get it.Thank you, Mr. Tang.

@SherlockShemol
Copy link
Contributor Author

This PR fixed #430 Previous comments can be found at #435

The concerns about compatibility are considered in this proposal. As mentioned in the scope, "Sedna joint inference and federated learning controller optimization only modifies the joint inference and federated learning controller codes. The controllers are decoupled from each other, and the function interface and library functions are not modified, so it will not affect the functions of other controllers. For existing application cases, since the CRD is not modified, the deployment of existing applications will not be affected."

The proposal looks good to me. We also got another lgtm at the routine meeting. It would be fine to merge now. However, the bug fix is not a trivial job. Sedna is also quite a complicated system. The proposal could be developed in a continuous manner and be modified during future implementations if needed.

Thank you, Mr. Zheng.I will continue to improve the proposal and carefully implement the solution.

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.

4 participants