Add Clone Snapshot Request Handling Scaffolding #63037
Conversation
Adds all the scaffolding for snapshot clone request handling and index-to-clone resolution to reduce the diff in elastic#61839 to the bare essentials of the state machine changes for snapshot cloning and relevant tests and give us the opportunity to review the API in isolation.
original-brownbear
added
>non-issue
:Distributed Coordination/Snapshot/Restore
|
Pinging @elastic/es-distributed (:Distributed/Snapshot/Restore) |
elasticmachine
added
the
Team:Distributed (Obsolete)
| private final String target; | ||
|
|
||
| // TODO: the current logic does not allow for specifying index resolution parameters like hidden and such. Do we care about cloning | ||
| // system or hidden indices? |
There was a problem hiding this comment.
Since hidden indices are included in snapshot by default (#57325) I think we should also allow them to be cloned and support IndicesOptions.
...c/main/java/org/elasticsearch/action/admin/cluster/snapshots/clone/CloneSnapshotRequest.java
Show resolved
Hide resolved
...c/main/java/org/elasticsearch/action/admin/cluster/snapshots/clone/CloneSnapshotRequest.java
Show resolved
Hide resolved
...java/org/elasticsearch/action/admin/cluster/snapshots/clone/CloneSnapshotRequestBuilder.java
Show resolved
Hide resolved
| * @return list of index ids to clone | ||
| * @throws SnapshotException on failure to find concrete request index ids or any index ids | ||
| */ | ||
| static List<IndexId> findIndexIdsToClone(SnapshotId sourceSnapshotId, Snapshot targetSnapshot, RepositoryData repositoryData, |
There was a problem hiding this comment.
Can't we just reuse or adapt filterIndices() so that restore and clone indices parameter work the same?
There was a problem hiding this comment.
🤦 yea let's do that and use IndicesOptions on the request, that makes it all fall into place much more nicely :) Thanks!
|
Thanks Tanguy, updated with your suggestions, this makes things a lot cleaner I think and also simplifies the other PR :) |
| @@ -659,6 +663,7 @@ public void initRestHandlers(Supplier<DiscoveryNodes> nodesInCluster) { | |||
| registerHandler.accept(new RestCleanupRepositoryAction()); | |||
| registerHandler.accept(new RestGetSnapshotsAction()); | |||
| registerHandler.accept(new RestCreateSnapshotAction()); | |||
| registerHandler.accept(new RestCloneSnapshotAction()); | |||
There was a problem hiding this comment.
Can we comment this for now and only uncomment it once REST integration tests and the whole feature are done?
There was a problem hiding this comment.
Mainly made this active here to make sure our RestController can handle the shared path with the create-snapshot action. Turned it only accepts this if the parameter names are the same across both actions so I left it in place to make sure we don't break anything.
I'm only going to merge this to master before final completion anyway, so maybe we can just leave it?
There was a problem hiding this comment.
Turned it only accepts this if the parameter names are the same across both actions so I left it in place to make sure we don't break anything.
Ok, I did not think about this. Let's keep it as it is and wait for the full feature to be done and merged before backporting it.
|
|
||
| private String[] indices; | ||
|
|
||
| private IndicesOptions indicesOptions = IndicesOptions.strictExpandOpen(); |
There was a problem hiding this comment.
Hidden indices are snapshotted by default so I think they should be cloned by default too? WDYT?
There was a problem hiding this comment.
I went with the defaults for restore here. I figured it's probably less likely that one would clone a hidden index so those seemed more natural?
There was a problem hiding this comment.
Actually I'm more concerned about the following use case:
create a snapshot of all indices (it includes hidden ones) -> clone this snapshot and remove some large index I don't care anymore (it also removes hidden indices right?) -> delete the previous snapshot as I now have a lighter snapshot (but the hidden indices are gone silently)
or maybe I'm missing something?
There was a problem hiding this comment.
Sold :), that's a conceivable use case for sure!
| out.writeString(repository); | ||
| out.writeString(source); | ||
| out.writeString(target); | ||
| out.writeStringArray(indices); |
|
Thanks again Tanguy, sorry for all the noise in this one! |
|
Jenkins run elasticsearch-ci/packaging-sample-windows |
|
Thank you Tanguy! |
Adds all the scaffolding for snapshot clone request handling and index-to-clone resolution to reduce the diff in elastic#61839 to the bare essentials of the state machine changes for snapshot cloning and relevant tests and give us the opportunity to review the API in isolation.
Adds all the scaffolding for snapshot clone request handling
and index-to-clone resolution to reduce the diff in #61839 to
the bare essentials of the state machine changes for snapshot
cloning and relevant tests and give us the opportunity to
review the API in isolation.