-
Notifications
You must be signed in to change notification settings - Fork 9.7k
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
Adding optional revision bump to snapshot restore #16029
Conversation
9cd9656
to
64cefc6
Compare
To clarify by |
With this latest update, I've changed the revision-bump flag to only increase the latest revision by the amount so that clients shouldn't see decreasing revisions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm (nonbinding)
etcdutl/snapshot/v3_snapshot.go
Outdated
) | ||
|
||
latest.main += amount | ||
k := [17]byte{} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Partially agree with @tjungblu . We should consider moving all revision related common functions (i.e. revToBytes, bytesToRev, newRevBytes) into package pkg
(or api
), so that they can be reused.
In this case, we should try to reuse newRevBytes. Please raise a followup ticket.
Signed-off-by: Allen Ray <alray@redhat.com>
I think that test failure may be a flake, running locally they succeeded. Can someone relaunch that test please? |
return latest | ||
} | ||
|
||
func (s *v3Manager) unsafeMarkRevisionCompacted(tx backend.BatchTx, latest revision) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Optional nit: it might be friendly if https://github.com/etcd-io/etcd/blob/main/server/storage/mvcc/kvstore.go#L201 can have a comment like "similar logic can be found in etcdctl side, this should be synced with the command"? Currently they share UnsafeSetScheduledCompact
so it's not easy to make them diverged. If we add additional method calls to server side, the comment will be helpful for not forgetting about the etcdutl side logic (such change might be quite unlikely though).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Thanks @dusk125
Two left actions:
|
Tickets have been raised. I'll be on holiday for the next two weeks, so I can handle them when I get back (July 17th). |
While @dusk125 is out, I can take over the 3.5 backport. Can anyone hit the merge button? Thanks. |
Signed-off-by: Wenjia Zhang <wenjiazhang@google.com>
Signed-off-by: Wenjia Zhang <wenjiazhang@google.com>
Signed-off-by: Wenjia Zhang <wenjiazhang@google.com>
This PR adds the flags
--bump-revision
and--mark-compacted
to etcdutl snapshot restore for #16028.I chose to handle this after the database is loaded from the snapshot, so as to not modify the snapshot, I didn't want to risk corrupting the snapshot as a part of this optional step. If for some reason the bump fails, a user would still be able to restore the snapshot again without the bump and restart the watchers manually.