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

[Refactor][RayCluster] Add RayClusterRedisPodAssociationOptions and HeadServiceAssociationOptions #2062

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

evalaiyc98
Copy link
Contributor

Why are these changes needed?

This PR aims to follow up #2045. The purpose of this initiative is to maintain consistency in association methods and avoid scattering MatchingLabels usage throughout the entire codebase.

In this PR, I mainly update the raycluster_controller.go file to reflect the change, and add two AssociationOptions which are RayClusterRedisPodAssociationOptions and HeadServiceAssociationOptions in association.go.

Related issue number

#2045

Checks

  • I've made sure the tests are passing.
  • Testing Strategy
    • Unit tests
    • Manual tests
    • This PR is not tested :(

@kevin85421
Copy link
Member

cc @rueian would you mind reviewing this PR? Thanks!

@kevin85421 kevin85421 self-requested a review April 5, 2024 20:39
@kevin85421 kevin85421 self-assigned this Apr 5, 2024
func HeadServiceAssociationOptions(instance *rayv1.RayCluster) AssociationOptions {
return AssociationOptions{
client.InNamespace(instance.Namespace),
client.MatchingLabels(HeadServiceLabels(*instance)),
Copy link
Contributor

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@kevin85421 kevin85421 left a comment

Choose a reason for hiding this comment

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

Please revisit all the function names.

ray-operator/controllers/ray/common/association.go Outdated Show resolved Hide resolved
ray-operator/controllers/ray/raycluster_controller.go Outdated Show resolved Hide resolved
ray-operator/controllers/ray/raycluster_controller.go Outdated Show resolved Hide resolved

if err := r.List(ctx, &services, client.InNamespace(instance.Namespace), filterLabels); err != nil {
if err := r.List(ctx, &services, common.RayClusterHeadPodsAssociationOptions(instance).ToListOptions()...); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

This is pretty confusing. We use RayClusterHeadPodsAssociationOptions to list service.

@evalaiyc98
Copy link
Contributor Author

Thank you! I have revised the function names to be more well-defined and try not to confuse developers.
If there is a need for any further improvement, please let me know.

@evalaiyc98 evalaiyc98 force-pushed the refactor-raycluster branch 2 times, most recently from 0d06e3f to 723a3d8 Compare April 29, 2024 12:33
@evalaiyc98
Copy link
Contributor Author

I intend to separate functions with similar functionality into RayClusterHeadPodsAssociationOptions and RayClusterServicesAssociationOptions, and try not to confuse developers.
How about you think @kevin85421 ?

@@ -103,6 +123,13 @@ func RayServiceRayClustersAssociationOptions(rayService *rayv1.RayService) Assoc
}
}

func HeadServiceAssociationOptions(instance *rayv1.RayCluster) AssociationOptions {
Copy link
Member

Choose a reason for hiding this comment

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

The function name is inconsistent with our naming convention.

@@ -84,7 +94,7 @@ func RayClusterGroupPodsAssociationOptions(instance *rayv1.RayCluster, group str
}
}

func RayClusterAllPodsAssociationOptions(instance *rayv1.RayCluster) AssociationOptions {
func RayClusterRelatedAssociationOptions(instance *rayv1.RayCluster) AssociationOptions {
Copy link
Member

Choose a reason for hiding this comment

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

What's the definition of "RayClusterRelated"?

@@ -93,6 +103,16 @@ func RayClusterAllPodsAssociationOptions(instance *rayv1.RayCluster) Association
}
}

func RayClusterServicesAssociationOptions(instance *rayv1.RayCluster) AssociationOptions {
Copy link
Member

Choose a reason for hiding this comment

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

What's the difference between this function and HeadServiceAssociationOptions?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants