Adds UUIDs to snapshots #18228
Adds UUIDs to snapshots #18228
Conversation
abeyad
added
>enhancement
review
:Distributed Coordination/Snapshot/Restore
| this.cause = cause; | ||
| this.name = name; | ||
| this.repository = repository; | ||
| public SnapshotRequest(String cause, Snapshot snapshot) { |
There was a problem hiding this comment.
While we are changing this constructor, can you put Snapshot as first argument?
There was a problem hiding this comment.
I have seen this comparison in several places. Maybe, it makes sense to move inside Snapshot and have something like boolean hasName(String repository, String name) method where it will take place? Not sure if hasName is a good name for this method though.
|
@ywelsch @imotov The latest commit addressing code review comments is here: a17039c Also, @imotov has requested I create an infrastructure for testing repositories with old snapshots interoperating with new snapshots. For now, the only thing to test is the blob names with respect to the changes, but it will make it easier to test changes down the road with generational index files. |
There was a problem hiding this comment.
This only loads the snapshots from disk.
Please fix and add a test.
There was a problem hiding this comment.
I think this comment has very limited shelf life :) It might make sense to omit the concrete versions since we are going to use this test after 5.0. Basically, say something about testing upgrading repository from the previous version to the new version. It might also make sense to rename the test to include word Upgraded since mixed is somewhat ambiguous here. Maybe RepositoryUpgradabilityIT?
|
LGTM |
|
Thank you for the review and valuable comments, @imotov! |
abeyad
force-pushed
the
snapshot-uuid
branch
2 times, most recently
from
16f6d2e to
23cb567
Compare
This commit adds a UUID for each snapshot, in addition to the already existing repository and snapshot name. The addition of UUIDs will enable more robust handling of the deletion of previous snapshots and lingering files from partially failed delete operations, on top of being able to uniquely track each snapshot. Relates elastic#18156
|
LGTM 2! Thanks for all the work here @abeyad. I think we've both learned quite a lot about some of the finer details of snap/restore. |
|
@ywelsch thank you for your patience and great feedback! Indeed we have learned a lot more on snapshot/resotre, which means more work ahead :) |
4 participants
This commit adds a UUID for each snapshot, in addition to the already
existing repository and snapshot name. The addition of UUIDs will enable
more robust handling of the deletion of previous snapshots and lingering
files from partially failed delete operations, on top of being able to
uniquely track each snapshot.
Relates #18156