Don't fail delete index API if an index is deleted during the request#138015
Don't fail delete index API if an index is deleted during the request#138015nielsbauman merged 4 commits intoelastic:mainfrom
Conversation
nielsbauman
added
>bug
:Data Management/Indices APIs
elasticsearchmachine
added
v9.3.0
Team:Data Management (obsolete)
|
Pinging @elastic/es-data-management (Team:Data Management) |
|
Hi @nielsbauman, I've created a changelog YAML for you. |
| byProject.computeIfAbsent(project.id(), ignore -> new HashSet<>()).add(index); | ||
| final Optional<ProjectMetadata> project = clusterState.metadata().lookupProject(index); | ||
| // In some cases, the index may have already been deleted by the time we process the deletion task. In that case, we can simply | ||
| // skip it. Index name resolution will have failed in the transport action (or in security) if the index never existed. |
There was a problem hiding this comment.
I think that the scenario is specifically when there's a wildcard delete request, and something else deletes the index between expanding the wildcard and doing the deletes, right? Maybe we could be more explicit about this here, I don't think I'd have realized that from this comment (I only got it from Mary's bug report).
There was a problem hiding this comment.
I thought about including that, but I figured that it's technically also possible with two concurrent delete requests (with concrete index names) that both resolve the same indices and then both trigger a cluster state update. But I understand that the situation from the bug report isn't immediately obvious, so I'll add a comment suggesting that as an example.
| if (project.isEmpty()) { | ||
| continue; | ||
| } | ||
| byProject.computeIfAbsent(project.get().id(), ignore -> new HashSet<>()).add(index); |
There was a problem hiding this comment.
Optional (no pun intended): I'm undecided whether a one-liner is more readable, or less:
project.map(ProjectMetadata::id).ifPresent(projectId -> byProject.computeIfAbsent(projectId, ignore -> new HashSet<>()).add(index));WDYT?
There was a problem hiding this comment.
I don't think I have a very strong opinion here either. I'm usually a fan of one-liners, but I kind of like the explicitness of the "skip" here.
There was a problem hiding this comment.
The purist would say that using Optional.get guarded by an ifPresent check is an anti-pattern, and we should always use things like ifPresent or orElse. But I'm not sure I agree... I'm happy enough with what you have here.
There was a problem hiding this comment.
Yeah, I usually prefer to avoid any nested code blocks when I can, as I think that generally makes things easier to read (but it doesn't always apply).
|
|
||
| for (final Map.Entry<ProjectId, Set<Index>> entry : byProject.entrySet()) { | ||
| // TODO Avoid creating the state multiple times if there are batched updates for multiple projects | ||
| clusterState = deleteIndices(clusterState.projectState(entry.getKey()), entry.getValue(), settings); |
There was a problem hiding this comment.
Thinking aloud: I was trying to figure out whether there's a small window in which the index could have been deleted between the check above and the deletion inside this method... I don't think there is, because that would have to be another cluster state update and those are serialized. The race condition only exists before the cluster state update starts. Does that sound right?
There was a problem hiding this comment.
Yep, that's exactly right. If we're inside a cluster state update task, we can be sure that no other cluster state is being processed/created.
3 participants
In some cases, an index may have already been deleted by the time we process the deletion cluster state update task. In that case, we can simply skip it instead of throwing an exception. Index name resolution will have failed in the transport action (or in security) if the index never existed.
Closes #137422