Add a method to export membership info to v2 store from RaftCluster #16132
Conversation
|
Overall looks good to me with a minor comment. |
|
Please also rebase this PR since it's far behind the head of main. |
|
@ahrtr @serathius thank you for the review. The change to not panic on keyNotFound is primarily driven by the new workflow to export membership to a new store (for snapshot). This store is empty and does not contain the members and will get a keyNotFound if a delete is attempted. The code is now updated to avoid changing mustDeletefromStore method. ptal. Thanks. |
|
cc @ptabor |
| @@ -167,32 +167,20 @@ func saveSnap(lg *zap.Logger, destSnap, srcSnap string, desired *desiredCluster) | |||
| walsnap.Index, walsnap.Term, walsnap.ConfState = snapshot.Metadata.Index, snapshot.Metadata.Term, &desired.confState | |||
| newss := snap.New(lg, destSnap) | |||
| snapshot.Metadata.ConfState = desired.confState | |||
| snapshot.Data = mustTranslateV2store(lg, snapshot.Data, desired) | |||
| snapshot.Data = mustTranslateV2store(lg, desired) | |||
There was a problem hiding this comment.
Why are we changing etcdutl backup command at all? It's a legacy deprecated way to create snapshot.
There was a problem hiding this comment.
This set of PRs is going towards removing the SetStore method of RaftCluster (along with usage of that store). The backup command is a consumer of SetStore so it needs to be updated.
If the backup command needs to be removed from code base, I can open another issue to get that done and inform the user base that the backup command is going away in 3.6. Please let me know if the backup command should be removed. /cc @ahrtr
There was a problem hiding this comment.
This set of PRs is going towards removing the SetStore method of RaftCluster (along with usage of that store). The backup command is a consumer of SetStore so it needs to be updated.
Agreed. We need to update the etcdutl backup command, otherwise we have to keep some source code (e.g. SetStore) which are used by etcdutl only.
Please let me know if the backup command should be removed. /cc @ahrtr
Let's discuss it separately. I tend to remove it.
Signed-off-by: Geeta Gharpure <[email protected]>
|
can this PR be merged if there is no additional review feedback? I would like to submit next PR which will consume changes from this one. |
|
cc @mitake and @wenjiaswe to take a look as well. |
|
It's unfortunate that there is no active review on a P0 item for 3.6. If there is no any additional review feedback by the end of this week. I will merge this PR by then given it's a safe change and there are already two approvals. |
Looks like there has been active review. Overall lgtm. @geetasg Thanks for working on this. I'll defer to @ahrtr and @serathius to decide on merging. |
|
thx @wenjiaswe for the review. |
Related to #12913
Introducing method to create the snapshot in v2 format using v3 state. This is used by the backup command in this PR. Main snapshot path will be updated in subsequent PRs.
Please read https://github.com/etcd-io/etcd/blob/main/CONTRIBUTING.md#contribution-flow.