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

change the replicas keyword to service_num #294

Closed
wants to merge 11 commits into from
Closed

change the replicas keyword to service_num #294

wants to merge 11 commits into from

Conversation

tiansuo114
Copy link
Contributor

change the replicas keyword in topoloy.yaml to service_num
but also keep the old version,old vversion topoloy.yaml still can use

other:
Change the replicas keyword displayed after executing the curveadm status command to service num

将 topoloy.yaml 中的replicas关键字更改为service_num
但也保留旧版本,旧版本的topoloy.yaml仍然可以使用

其他:
将执行curveadm status命令后显示的replicas关键字改为service num

@Wine93
Copy link
Collaborator

Wine93 commented Aug 20, 2023

I think maybe instance is more better, it's more meaningful and easy to understand.

@Wine93 Wine93 self-requested a review August 20, 2023 03:06
tiansuo114 and others added 8 commits August 21, 2023 17:00
更改Replicas关键字,更新curvebs的topology.yaml

Signed-off-by: tec-rubbish maker <[email protected]>
Signed-off-by: tec-rubbish maker <[email protected]>
Signed-off-by: tec-rubbish maker <[email protected]>
Signed-off-by: tec-rubbish maker <[email protected]>
Signed-off-by: tec-rubbish maker <[email protected]>
Signed-off-by: tec-rubbish maker <[email protected]>
Signed-off-by: tec-rubbish maker <[email protected]>
Signed-off-by: tec-rubbish maker <[email protected]>
Signed-off-by: tec-rubbish maker <[email protected]>
Signed-off-by: tec-rubbish maker <[email protected]>
Signed-off-by: tec-rubbish maker <[email protected]>
Signed-off-by: tec-rubbish maker <[email protected]>
Signed-off-by: tec-rubbish maker <[email protected]>
Signed-off-by: tec-rubbish maker <[email protected]>
Signed-off-by: tec-rubbish maker <[email protected]>
@tiansuo114
Copy link
Contributor Author

I think maybe instance is more better, it's more meaningful and easy to understand.

Okay, I have completed the code change (that is, changed the service_num keyword to instance) and committed the code. Please help conduct a code review for my code.

@@ -152,7 +152,7 @@ func checkMigrateTopology(curveadm *cli.CurveAdm, data string) error {
dcs2add[0].GetRole() != dcs2del[0].GetRole() {
return errno.ERR_REQUIRE_SAME_ROLE_SERVICES_FOR_MIGRATING
}
if len(dcs2del) != dcs2del[0].GetReplicas() {
if len(dcs2del) != dcs2del[0].GetInstance() {
Copy link
Collaborator

@Wine93 Wine93 Aug 27, 2023

Choose a reason for hiding this comment

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

Sorry, i didn't express clearly, actually we should use word instances for multi-instances, so GetInstances not GetInstance.

type statusOptions struct {
id string
role string
host string
verbose bool
showReplicas bool
showInstance bool
Copy link
Collaborator

Choose a reason for hiding this comment

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

ditto.

ERR_REPLICAS_REQUIRES_POSITIVE_INTEGER = EC(331002, "replicas requires a positive integer")
ERR_INVALID_VARIABLE_SECTION = EC(331003, "invalid variable section")
ERR_DUPLICATE_SERVICE_ID = EC(331004, "service id is duplicate")
//todo: remove ERR_REPLICAS_REQUIRES_POSITIVE_INTEGER and change to ERR_NO_SERVICES_NUM_TOPOLOGY
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should update the error code which show below.

@Wine93
Copy link
Collaborator

Wine93 commented Aug 27, 2023

BTW: please rebase all commits into one when all is ready.

@Wine93 Wine93 added this to the v0.3.0 milestone Aug 27, 2023
@tiansuo114
Copy link
Contributor Author

I think i make a mistake in pull request😢😢
So i decide to make a new pull request,sorry to border

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

Successfully merging this pull request may close these issues.

3 participants