Remove Snapshot INIT Step (#55918)#59374
Remove Snapshot INIT Step (#55918)#59374original-brownbear merged 3 commits intoelastic:7.xfrom original-brownbear:59918-7.x
Conversation
With #55773 the snapshot INIT state step has become obsolete. We can set up the snapshot directly in one single step to simplify the state machine. This is a big help for building concurrent snapshots because it allows us to establish a deterministic order of operations between snapshot create and delete operations since all of their entries now contain a repository generation. With this change simple queuing up of snapshot operations can and will be added in a follow-up.
|
Pinging @elastic/es-distributed (:Distributed/Snapshot/Restore) |
|
Jenkins run elasticsearch-ci/packaging-sample-windows (dep download issue) |
|
Jenkins run elasticsearch-ci/2 (known GCS ITs infra issue) |
| * @param listener snapshot completion listener | ||
| */ | ||
| public void executeSnapshot(final CreateSnapshotRequest request, final ActionListener<SnapshotInfo> listener) { | ||
| public void executeSnapshotLegacy(final CreateSnapshotRequest request, final ActionListener<SnapshotInfo> listener) { |
There was a problem hiding this comment.
This and the other *Legacy method rename (and the way they're selectively called from the transport action) is the only real difference between this commit and the original one that went into master. I realize that it is a little dirty to just duplicate everything and keep the old path code unchanged but IMO this is the easiest way to bring in the remaining snapshot back-ports cleanly.
ywelsch
left a comment
There was a problem hiding this comment.
I've left one important comment. Can we dry up some of the logic that's shared by both methods in a follow-up (and do the same on master branch)? I'm generally fine with the approach but worried a bit about testing coverage of that old code branch.
| */ | ||
| public void executeSnapshot(final CreateSnapshotRequest request, final ActionListener<SnapshotInfo> listener) { | ||
| public void executeSnapshotLegacy(final CreateSnapshotRequest request, final ActionListener<SnapshotInfo> listener) { | ||
| createSnapshot(request, |
There was a problem hiding this comment.
Should this be createSnapshotLegacy?
Is this covered by testing?
There was a problem hiding this comment.
This is covered by mixed cluster tests yes. createSnapshotLegacy is already taken for the non-blocking path, I just figured I'd stick with the naming from the original methods for both here.
There was a problem hiding this comment.
I'm confused. Why is the executeSnapshotLegacy method not delegating to createSnapshotLegacy?
There was a problem hiding this comment.
🤦 ok now I get it. Sorry. Sadly we don't have a test that would've caught the issue from this mistake and I don't see an easy way to add one. What we'd need is a BwC disruption test where we fail over from new version to old version master while the snapshot is in progress. I'm starting to think we should have that but I don't see a quick way of setting this up since all the BwC tests are rest tests right now and we don't have any way of using the mock repository with those.
=> Just fixed it for now.
First off, this code isn't part of the As for drying up. I actually wanted to specifically avoid drying things up here precisely because |
|
Thanks Yannick! |
With #55773 the snapshot INIT state step has become obsolete. We can set up the snapshot directly in one single step to simplify the state machine.
This is a big help for building concurrent snapshots because it allows us to establish a deterministic order of operations between snapshot create and delete operations since all of their entries now contain a repository generation. With this change simple queuing up of snapshot operations can and will be added in a follow-up.
backport of #55918