Repository Cleanup Endpoint#43900
Repository Cleanup Endpoint#43900original-brownbear merged 105 commits intoelastic:masterfrom original-brownbear:cleanup-repo-ep
Conversation
tlrx
left a comment
There was a problem hiding this comment.
Thanks for the changes, I liked it better without the LongConsumer. Can we decide on the response output? That's the last item that prevents me to LGTM ;)
Also, did you consider to have a dry-run option?
|
|
||
| public static final DeleteResult ZERO = new DeleteResult(0, 0); | ||
|
|
||
| private final long blobsDeleted; |
There was a problem hiding this comment.
nit: this could be just blobs & bytes
There was a problem hiding this comment.
Hmm, I'd rather not, then I have to use this in add :D
...r/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestCleanupRepositoryAction.java
Outdated
Show resolved
Hide resolved
|
Thanks @tlrx
Np, so do I :D
I like the one level of nesting better but have no strong arguments for it other than maybe consistency with other APIs and extensibility.
Yea but in the end I'm not sure I see the point. If you think about it, all you can present to the user is a bunch of uuids for indices and stale root blobs). What can they really get out of that? |
|
Jenkins run elasticsearch-ci/bwc |
2 similar comments
|
Jenkins run elasticsearch-ci/bwc |
|
Jenkins run elasticsearch-ci/bwc |
|
Jenkins run elasticsearch-ci/packaging-sample |
|
@tlrx ping :) (no rush I know you just returned)
I still like the current format :) ... mainly for its easier extensibility. If that's the last thing blocking this maybe we can go with the current version and merge this? :) |
tlrx
left a comment
There was a problem hiding this comment.
LGTM
Concerning the response output I expressed my opinion but if both you and @andrershov prefer the proposed format then I'm fine :)
Thanks for the response on the dry-run option, I wanted to know if it was considered at some point. Since this is something that can be added later, we'll see if someone requests it.
|
Thanks so much @tlrx and @andrershov for reviewing this big one! |
* Repository Cleanup Endpoint (#43900) * Snapshot cleanup functionality via transport/REST endpoint. * Added all the infrastructure for this with the HLRC and node client * Made use of it in tests and resolved relevant TODO * Added new `Custom` CS element that tracks the cleanup logic. Kept it similar to the delete and in progress classes and gave it some (for now) redundant way of handling multiple cleanups but only allow one * Use the exact same mechanism used by deletes to have the combination of CS entry and increment in repository state ID provide some concurrency safety (the initial approach of just an entry in the CS was not enough, we must increment the repository state ID to be safe against concurrent modifications, otherwise we run the risk of "cleaning up" blobs that just got created without noticing) * Isolated the logic to the transport action class as much as I could. It's not ideal, but we don't need to keep any state and do the same for other repository operations (like getting the detailed snapshot shard status)
CustomCS element that tracks the cleanup logic. Kept it similar to the delete and in progress classes and gave it some (for now) redundant way of handling multiple cleanups but only allow one