Skip to content
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

Repository Cleanup Endpoint #43900

Merged
merged 105 commits into from
Aug 21, 2019

Conversation

original-brownbear
Copy link
Member

@original-brownbear original-brownbear commented Jul 3, 2019

  • 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)

@original-brownbear
Copy link
Member Author

Thanks @tlrx

Thanks for the changes, I liked it better without the LongConsumer.

Np, so do I :D

Can we decide on the response output? That's the last item that prevents me to LGTM ;)

I like the one level of nesting better but have no strong arguments for it other than maybe consistency with other APIs and extensibility.

Also, did you consider to have a dry-run option?

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?
On the other hand, the response could be problematically huge in some cases and if we add more logic here to do some deep cleaning of shards it gets questionable how we would even present the results of that (IMO) -> not sure it's worth it unless someone sees a good use case?

@original-brownbear original-brownbear requested a review from tlrx August 9, 2019 17:02
@original-brownbear
Copy link
Member Author

Jenkins run elasticsearch-ci/bwc

2 similar comments
@original-brownbear
Copy link
Member Author

Jenkins run elasticsearch-ci/bwc

@original-brownbear
Copy link
Member Author

Jenkins run elasticsearch-ci/bwc

Copy link
Contributor

@andrershov andrershov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@original-brownbear
Copy link
Member Author

Jenkins run elasticsearch-ci/packaging-sample

@original-brownbear
Copy link
Member Author

@tlrx ping :) (no rush I know you just returned)

Can we decide on the response output? That's the last item that prevents me to LGTM ;)

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? :)

Copy link
Member

@tlrx tlrx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@original-brownbear
Copy link
Member Author

Thanks so much @tlrx and @andrershov for reviewing this big one!

@original-brownbear original-brownbear merged commit df01766 into elastic:master Aug 21, 2019
@original-brownbear original-brownbear deleted the cleanup-repo-ep branch August 21, 2019 10:02
original-brownbear added a commit to original-brownbear/elasticsearch that referenced this pull request Aug 21, 2019
* 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)
original-brownbear added a commit to original-brownbear/elasticsearch that referenced this pull request Aug 21, 2019
Disabling BwC tests so elastic#45780 can be merged
original-brownbear added a commit that referenced this pull request Aug 21, 2019
Disabling BwC tests so #45780 can be merged
original-brownbear added a commit that referenced this pull request Aug 21, 2019
* 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)
@original-brownbear original-brownbear restored the cleanup-repo-ep branch August 6, 2020 18:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants