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

feat:add bs query snapshot #2943

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

Conversation

ZackSoul
Copy link
Contributor

@ZackSoul ZackSoul commented Dec 4, 2023

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

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?

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

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


```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.


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


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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants