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

Curvebs stop volume snapshot #2794

Merged
merged 1 commit into from
Nov 14, 2023
Merged

Curvebs stop volume snapshot #2794

merged 1 commit into from
Nov 14, 2023

Conversation

ZackSoul
Copy link
Contributor

@ZackSoul ZackSoul commented Oct 11, 2023 •
edited
Loading

What problem does this PR solve?

Issue Number: #2579

Problem Summary:

What is changed and how it works?

add curve bs stop volume snapshot

What's Changed:

How it Works:
curve bs stop volume volumeSnapshot
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

cicheck

23 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

@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

@Cyber-SiKu
Copy link
Contributor

cicheck

@Cyber-SiKu
Copy link
Contributor

cicheck

3 similar comments
@ZackSoul
Copy link
Contributor Author

cicheck

@ZackSoul
Copy link
Contributor Author

cicheck

@ZackSoul
Copy link
Contributor Author

cicheck

caoxianfei1
caoxianfei1 reviewed Oct 23, 2023
View reviewed changes
test/tools/status_tool_test.cpp Outdated
// EXPECT_CALL(*mdsClient_, ListChunkServersInCluster(
// An<std::map<PoolIdType, std::vector<ChunkServerInfo>>*>()))
// .Times(1)
// .WillOnce(Return(-1));
ASSERT_EQ(-1, statusTool.RunCommand("chunkserver-status"));
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Now tool code will not run the ci, so you can revert this change

caoxianfei1
caoxianfei1 reviewed Oct 23, 2023
View reviewed changes
tools-v2/internal/error/error.go Outdated
@@ -481,6 +481,12 @@ var (
ErrListWarmup = func() *CmdError {
return NewInternalCmdError(74, "list warmup progress fail, err: %s")
}
ErrBsGetSnapShotListResult = func() *CmdError {
Copy link
Contributor

@caoxianfei1 caoxianfei1 Oct 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

not be used?

caoxianfei1
caoxianfei1 reviewed Oct 23, 2023
View reviewed changes
tools-v2/pkg/cli/command/curvebs/stop/volumeSnapshot/stop_volume.go Outdated
return records, err
}

func (s *Stop) getSnapShotListAll() ([]*Record, *cmderror.CmdError) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The function is intend to list all snapshot? may be we can provide a command such as curve bs list snapshot, then we invoke the interface here.

caoxianfei1
caoxianfei1 reviewed Oct 23, 2023
View reviewed changes
tools-v2/pkg/cli/command/curvebs/stop/volumeSnapshot/stop_volume.go Outdated
Comment on lines 13 to 17
const (
VERSION = "0.0.6"
LIMIT = "100"
OFFSET = "0"
)
Copy link
Contributor

Choose a reason for hiding this comment

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

The variable place global floder is better? because all snapshot command wiil use it like NewSubUri function.

caoxianfei1
caoxianfei1 reviewed Oct 23, 2023
View reviewed changes
tools-v2/pkg/config/bs.go Outdated
@@ -142,6 +142,8 @@ const (
VIPER_CURVEBS_DEST = "curvebs.dest"
CURVEBS_TASKID = "taskid"
VIPER_CURVEBS_TASKID = "curvebs.taskid"
CURVEBS_SNAPSHOTSEQID = "snapshotseqid"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that the option name is too long.

caoxianfei1
caoxianfei1 reviewed Oct 23, 2023
View reviewed changes
tools-v2/pkg/cli/command/curvebs/stop/volumeSnapshot/stop_volume.go Outdated
OFFSET = "0"
)

type Stop struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

The Stop struct name is inappropriate. Change another name?

@ZackSoul ZackSoul force-pushed the new-curvebs-stop-snapshot branch 2 times, most recently from 7e837a7 to 4c17129 Compare November 2, 2023 16:51
@ZackSoul
Copy link
Contributor Author

ZackSoul commented Nov 3, 2023

cicheck

8 similar comments
@Cyber-SiKu
Copy link
Contributor

cicheck

@Cyber-SiKu
Copy link
Contributor

cicheck

@ZackSoul
Copy link
Contributor Author

ZackSoul commented Nov 3, 2023

cicheck

@ZackSoul
Copy link
Contributor Author

ZackSoul commented Nov 3, 2023

cicheck

@ZackSoul
Copy link
Contributor Author

ZackSoul commented Nov 3, 2023

cicheck

@ZackSoul
Copy link
Contributor Author

ZackSoul commented Nov 3, 2023

cicheck

@ZackSoul
Copy link
Contributor Author

ZackSoul commented Nov 3, 2023

cicheck

@ZackSoul
Copy link
Contributor Author

ZackSoul commented Nov 3, 2023

cicheck

@ZackSoul
Copy link
Contributor Author

ZackSoul commented Nov 3, 2023

@caoxianfei1 I've refactored it with listsnapshot, waiting for further review.

@ZackSoul ZackSoul force-pushed the new-curvebs-stop-snapshot branch 5 times, most recently from a5f4867 to c6b32b4 Compare November 6, 2023 01:43
@ZackSoul
add bs stop snapshot
291e3c9 
Signed-off-by: ZackSoul <[email protected]>

add bs stop snapshot

Signed-off-by: ZackSoul <[email protected]>

add bs stop snapshot

Signed-off-by: ZackSoul <[email protected]>

add bs stop snapshot

Signed-off-by: ZackSoul <[email protected]>

add bs stop snapshot

Signed-off-by: ZackSoul <[email protected]>

add bs stop snapshot

Signed-off-by: ZackSoul <[email protected]>

add bs stop snapshot

Signed-off-by: ZackSoul <[email protected]>

add bs stop snapshot

Signed-off-by: ZackSoul <[email protected]>
@ZackSoul
Copy link
Contributor Author

ZackSoul commented Nov 6, 2023

cicheck

@ZackSoul
Copy link
Contributor Author

ZackSoul commented Nov 6, 2023

call for rerview @caoxianfei1

@ZackSoul
Copy link
Contributor Author

waiting for review

@caoxianfei1 caoxianfei1 merged commit b134e88 into opencurve:master Nov 14, 2023
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Cyber-SiKu Cyber-SiKu Cyber-SiKu 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 @Cyber-SiKu @caoxianfei1