-
Notifications
You must be signed in to change notification settings - Fork 25.2k
Implement dangling indices API #50920
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
Implement dangling indices API #50920
Conversation
Pinging @elastic/es-distributed (:Distributed/Recovery) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this. I've left some comments and items to discuss.
We should break this into smaller reviewable chunks once we agree on the direction.
rest-api-spec/src/main/resources/rest-api-spec/api/dangling_indices.delete.json
Outdated
Show resolved
Hide resolved
rest-api-spec/src/main/resources/rest-api-spec/api/dangling_indices.delete.json
Outdated
Show resolved
Hide resolved
rest-api-spec/src/main/resources/rest-api-spec/api/dangling_indices.delete.json
Outdated
Show resolved
Hide resolved
rest-api-spec/src/main/resources/rest-api-spec/api/dangling_indices.restore.json
Outdated
Show resolved
Hide resolved
...java/org/elasticsearch/action/admin/indices/dangling/TransportDeleteDanglingIndexAction.java
Outdated
Show resolved
Hide resolved
...java/org/elasticsearch/action/admin/indices/dangling/TransportDeleteDanglingIndexAction.java
Outdated
Show resolved
Hide resolved
...java/org/elasticsearch/action/admin/indices/dangling/TransportDeleteDanglingIndexAction.java
Outdated
Show resolved
Hide resolved
...java/org/elasticsearch/action/admin/indices/dangling/TransportDeleteDanglingIndexAction.java
Outdated
Show resolved
Hide resolved
clusterService.addListener(this); | ||
} else { | ||
logger.warn(AUTO_IMPORT_DANGLING_INDICES_SETTING.getKey() + " is disabled, dangling indices will not be detected or imported"); | ||
clusterService.addListener(this); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This means that dangling indices scanning is still done on every cluster state update. One of the benefits of this PR was that this now would only needed to be done on-demand, i.e. when a user requests the list of dangling indices, not on every CS update.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This still needs addressing. I agree that we can hunt for dangling indices on demand for the List
action, and also load just the specific metadata needed for the Find
action. The DanglingIndicesState
is effectively a cache, but I don't think we need to cache anything.
We could follow-up with something that checks for dangling indices at startup or periodically if we feel it's needed.
Refer to "importing" dangling indices rather than "restoring" them. Also keeping UUID uppercased in various places, instead of camel-cased.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @pugnascotia, I left a handful of small comments. Apart from that I'm 100% ok with the new APIs and all that infrastructure. I'll need another opinion on the changes to the existing code as I'm finding it very hard to follow all the different states that we can be in while deleting an index.
server/src/main/java/org/elasticsearch/cluster/ClusterChangedEvent.java
Outdated
Show resolved
Hide resolved
...org/elasticsearch/action/admin/indices/dangling/import_index/ImportDanglingIndexRequest.java
Outdated
Show resolved
Hide resolved
...a/org/elasticsearch/action/admin/indices/dangling/list/ListDanglingIndicesResponseTests.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/gateway/DanglingIndicesState.java
Outdated
Show resolved
Hide resolved
server/src/test/java/org/elasticsearch/gateway/DanglingIndicesStateTests.java
Show resolved
Hide resolved
server/src/test/java/org/elasticsearch/gateway/DanglingIndicesStateTests.java
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/gateway/DanglingIndicesState.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/gateway/DanglingIndicesState.java
Outdated
Show resolved
Hide resolved
...g/elasticsearch/action/admin/indices/dangling/delete/TransportDeleteDanglingIndexAction.java
Show resolved
Hide resolved
By extending AcknowledgedRequest, it becomes possible to replace ImportDanglingIndexResponse with AcknowledgedResponse.
After talking to @DaveCTurner, I've removed the log line in question, as well as fix a test break. I believe there are now no outstanding issues on this PR. |
@DaveCTurner @ywelsch is there anything else you'd like to see changed on this PR? |
Once this is merged, we should also add the LIST API to the support diagnostics tool. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @pugnascotia, this LGTM :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've only checked the core changes in IndicesClusterStateService
, ClusterChangedEvent
and DanglingIndicesState
, which looked good to me.
Backport of #50920. Part of #48366. Implement an API for listing, importing and deleting dangling indices. Co-authored-by: David Turner <[email protected]>
Part of #48366. Implement an API for managing dangling indices.
I've been staring at this code for a while so I'd really appreciate some feedback. Note that I haven't (yet) implemented wildcard restores, or written API documentation.