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

feat:add bs query snapshot #2943

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

feat:add bs query snapshot #2943

wants to merge 1 commit into from

Conversation

ZackSoul
Copy link
Contributor

@ZackSoul ZackSoul commented Dec 4, 2023 •
edited
Loading

What problem does this PR solve?

Issue Number: #2582

Problem Summary:

What is changed and how it works?

add curve bs query snapshot

What's Changed:

How it Works:
query测试

Side effects(Breaking backward compatibility? Performance regression?):

Check List

  • Relevant documentation/comments is changed or added
  • I acknowledge that all my contributions will be made under the project's license

@ZackSoul
Copy link
Contributor Author

ZackSoul commented Dec 4, 2023

cicheck

2 similar comments
@ZackSoul
Copy link
Contributor Author

ZackSoul commented Dec 6, 2023

cicheck

@ZackSoul
Copy link
Contributor Author

ZackSoul commented Dec 6, 2023

cicheck

caoxianfei1
caoxianfei1 reviewed Dec 12, 2023
View reviewed changes
tools-v2/pkg/cli/command/curvebs/query/snapshot/snapshot.go Outdated
Comment on lines 142 to 151
row[cobrautil.ROW_STATUS] = fmt.Sprintf("%d", item.Status)
row[cobrautil.ROW_SNAPSHOT_SEQNUM] = fmt.Sprintf("%d", item.SeqNum)
row[cobrautil.ROW_FILE_LENGTH] = fmt.Sprintf("%d", item.FileLength)
row[cobrautil.ROW_PROGRESS] = fmt.Sprintf("%v", item.Progress)
row[cobrautil.ROW_CREATE_TIME] = time.Unix(int64(item.Time/1000000), 0).Format("2006-01-02 15:04:05")
Copy link
Contributor

Choose a reason for hiding this comment

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

convert int to string has better method?

caoxianfei1
caoxianfei1 approved these changes Dec 12, 2023
View reviewed changes
Copy link
Contributor

@caoxianfei1 caoxianfei1 left a comment

Choose a reason for hiding this comment

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

LGTM!

@ZackSoul
Copy link
Contributor Author

cicheck

@ZackSoul
Copy link
Contributor Author

cicheck

13 similar comments
@caoxianfei1
Copy link
Contributor

cicheck

@ZackSoul
Copy link
Contributor Author

cicheck

@ZackSoul
Copy link
Contributor Author

cicheck

@ZackSoul
Copy link
Contributor Author

cicheck

@ZackSoul
Copy link
Contributor Author

cicheck

@caoxianfei1
Copy link
Contributor

cicheck

@caoxianfei1
Copy link
Contributor

cicheck

@ZackSoul
Copy link
Contributor Author

cicheck

@ZackSoul
Copy link
Contributor Author

cicheck

@ZackSoul
Copy link
Contributor Author

cicheck

@ZackSoul
Copy link
Contributor Author

cicheck

@ZackSoul
Copy link
Contributor Author

cicheck

@ZackSoul
Copy link
Contributor Author

cicheck

@caoxianfei1 caoxianfei1 reopened this Dec 19, 2023
@caoxianfei1
Copy link
Contributor

cicheck

1 similar comment
@ZackSoul
Copy link
Contributor Author

cicheck

@caoxianfei1 caoxianfei1 reopened this Dec 20, 2023
@caoxianfei1
Copy link
Contributor

cicheck

1 similar comment
@ZackSoul
Copy link
Contributor Author

cicheck

@ZackSoul
Copy link
Contributor Author

cicheck

14 similar comments
@ZackSoul
Copy link
Contributor Author

cicheck

@ZackSoul
Copy link
Contributor Author

cicheck

@ZackSoul
Copy link
Contributor Author

cicheck

@ZackSoul
Copy link
Contributor Author

cicheck

@ZackSoul
Copy link
Contributor Author

cicheck

@ZackSoul
Copy link
Contributor Author

cicheck

@ZackSoul
Copy link
Contributor Author

cicheck

@ZackSoul
Copy link
Contributor Author

cicheck

@ZackSoul
Copy link
Contributor Author

cicheck

@ZackSoul
Copy link
Contributor Author

cicheck

@ZackSoul
Copy link
Contributor Author

cicheck

@ZackSoul
Copy link
Contributor Author

cicheck

@ZackSoul
Copy link
Contributor Author

cicheck

@montaguelhz
Copy link
Contributor

cicheck

montaguelhz
montaguelhz reviewed Jan 5, 2024
View reviewed changes
Copy link
Contributor

@montaguelhz montaguelhz 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

tools-v2/README.md Outdated

```shell
curve bs query snapshot --path /test/test111 --snapshotstatus done
(other status: "in-progess" "deleting" "errorDeleteing" "canceling" "error")
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe ## is better than ().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your comment, but I don't understand the ##, any example?

Copy link
Contributor

Choose a reason for hiding this comment

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

for example,

curve bs query snapshot --path /test/test111 --snapshotstatus done # other status: "in-progess" "deleting" "errorDeleteing" "canceling" "error"

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like done is a keyword in shell, is that a problem？

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems to be fine in the tests, and done should work only with for or while etc.

tools-v2/pkg/cli/command/curvebs/query/snapshot/snapshot.go Outdated

const (
snapshotExample = `$ curve bs query snapshot --path /test/test111 --snapshotstatus done
(other status: "in-progress" "deleting" "errorDeleteing" "canceling" "error")`
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto. And extra space will be shown if call help.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image
Can I do like this to sovle the extar space?

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PTAL, thanks@montaguelhz

@ZackSoul
Copy link
Contributor Author

cicheck

2 similar comments
@ZackSoul
Copy link
Contributor Author

cicheck

@ZackSoul
Copy link
Contributor Author

cicheck

montaguelhz
montaguelhz reviewed Jan 11, 2024
View reviewed changes
tools-v2/pkg/cli/command/curvebs/query/snapshot/snapshot.go Outdated

const (
snapshotExample = `$ curve bs query snapshot --path /test/test111 --snapshotstatus done
(other status: "in-progress" "deleting" "errorDeleteing" "canceling" "error")`
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, maybe I didn't make myself clear, please keep the README and the example consistent, all use #. this is the last suggestion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Understand, that has been corrected.

@ZackSoul
Copy link
Contributor Author

cicheck

2 similar comments
@ZackSoul
Copy link
Contributor Author

cicheck

@montaguelhz
Copy link
Contributor

cicheck

@ZackSoul
Copy link
Contributor Author

cicheck

1 similar comment
@ZackSoul
Copy link
Contributor Author

cicheck

@montaguelhz
Copy link
Contributor

@caoxianfei1 it can be merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@montaguelhz montaguelhz montaguelhz approved these changes

@caoxianfei1 caoxianfei1 caoxianfei1 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ZackSoul @caoxianfei1 @montaguelhz