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] refactoring code to remove duplicate code #152

Merged
merged 2 commits into from
Jul 3, 2023

Conversation

yandongxiao
Copy link
Collaborator

No description provided.

Copy link
Collaborator

@kevincai kevincai left a comment

Choose a reason for hiding this comment

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

TBD

Makefile Outdated Show resolved Hide resolved
@@ -1064,7 +1064,7 @@ spec:
type: object
type: array
image:
description: Image for a starrocks be deployment.
description: Image for a starrocks deployment..
Copy link
Collaborator

Choose a reason for hiding this comment

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

redundant dot

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

}

// StarRocksCnSpec defines the desired state of cn.
type StarRocksCnSpec struct {
Copy link
Collaborator

Choose a reason for hiding this comment

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

may still needs StorageVolumes for CN, e.g, mount volumes for log or for cache

// to be owned by the pod:
FsGroup *int64 `json:"fsGroup,omitempty"`

//Replicas is the number of desired Pod, the number is 1,3,5
Copy link
Collaborator

Choose a reason for hiding this comment

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

the number is 1,3,5 is misleading, remove it.

}

func (spec *StarRocksBeSpec) GetStorageVolumes() []StorageVolume {
return spec.StorageVolumes
Copy link
Collaborator

Choose a reason for hiding this comment

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

when need to check if the spec is nil, when not needed? what's the criteria?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is a very good question! Currently, although there is no check for whether spec *StarRocksBeSpec is nil, the SubController, during reconciling, will first verify if their respective Spec is nil.

},
{
Name: "USER",
Value: "root",
Copy link
Collaborator

Choose a reason for hiding this comment

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

// TODO: make this configurable.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So, USER==root is its default value?

Copy link
Collaborator

Choose a reason for hiding this comment

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

yes, I mean add a TODO here, we will soon go back here to support non-root mode.

@yandongxiao
Copy link
Collaborator Author

I have submitted a new commit based on the feedback provided in the review above.

@kevincai kevincai merged commit bba77ff into StarRocks:main Jul 3, 2023
2 checks passed
@yandongxiao yandongxiao added the refactor refactor code label Jul 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor refactor code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants