Bulk cluster state updates on index deletion #11258
Conversation
clintongormley
changed the title
|
@s1monw could you review this please? |
|
hey, this has not been forgotten... I wonder if we do need to do some higher level refactorings to this area of the code. I will look into it next week and come back to this. |
|
@bogensberger I finally found the time to look closer at this. It always bugged me that we had a list of semaphores to acquire and maintain across the lifetime of the delete. This is super error prone and was the main reason why we didn't move forward here sooner. I spend the day looking into why it is needed and what the reasons were why it was added (in 0.17.0) ;) ie. 200 years ago... anyway I am sure we fixed all the problems that lead to the addition of these semaphores in other PRs in 1.x already so we can remove the locking and just go ahead without the locks. I opened #14159 to get rid of it which I will port for 2.2 once it's in would you mind bringing this up to date? |
|
@s1monw yes, i will do. |
|
@bogensberger I pushed it to master... please go ahead! |
|
@s1monw thanks, i've updated the PR |
|
it looks good to me @javanna can you please take a look at this as well |
| Collection<String> indices = Arrays.asList(request.indices); | ||
| final DeleteIndexListener deleteListener = new DeleteIndexListener(userListener); | ||
|
|
||
| clusterService.submitStateUpdateTask("delete-index [" + indices + "]", Priority.URGENT, new ClusterStateUpdateTask() { |
There was a problem hiding this comment.
I think we might end up with double square brackets here given that the string representation of the list will be like
"[index1, index2, index3]" already.
|
I had a quick look at this and left a few minor comments, looks very good though. I might be getting confused, but I think this one makes #11189 obsolete, and it's a very good change, long overdue. @bogensberger do you have time to address those minor comments so we can pull this in? |
|
@bogensberger if you don't have time to apply the changes I can do it... but I'd love if you could do it :) |
|
@s1monw i will do it! |
|
@bogensberger awesome thanks! |
|
looks great thanks @bogensberger ! |
…n_index_deletion Bulk cluster state updates on index deletion
|
merged into master - I will port to 2.x after it's seen some CI cycles thanks @bogensberger |
…dates_on_index_deletion Bulk cluster state updates on index deletion
This commit fixes a regression introduced with elastic#12058. This causes failures with the delete index api when providing the same index name multiple times in the request, or aliases/wildcard expressions that end up pointing to the same concrete index. The bug was revealed after merging elastic#11258 as we delete indices in batch rather than one by one. The master node will expect too many acknowledgements based on the number of indices that it's trying to delete, hence the request will never be acknowledged by all nodes. Closes elastic#14316
This commit fixes a regression introduced with elastic#12058. This causes failures with the delete index api when providing the same index name multiple times in the request, or aliases/wildcard expressions that end up pointing to the same concrete index. The bug was revealed after merging elastic#11258 as we delete indices in batch rather than one by one. The master node will expect too many acknowledgements based on the number of indices that it's trying to delete, hence the request will never be acknowledged by all nodes. Closes elastic#14316
This commit fixes a regression introduced with elastic#12058. This causes failures with the delete index api when providing the same index name multiple times in the request, or aliases/wildcard expressions that end up pointing to the same concrete index. The bug was revealed after merging elastic#11258 as we delete indices in batch rather than one by one. The master node will expect too many acknowledgements based on the number of indices that it's trying to delete, hence the request will never be acknowledged by all nodes. Closes elastic#14316
This commit fixes a regression introduced with elastic#12058. This causes failures with the delete index api when providing the same index name multiple times in the request, or aliases/wildcard expressions that end up pointing to the same concrete index. The bug was revealed after merging elastic#11258 as we delete indices in batch rather than one by one. The master node will expect too many acknowledgements based on the number of indices that it's trying to delete, hence the request will never be acknowledged by all nodes. Closes elastic#14316
clintongormley
added
:Distributed/Distributed
This fix is related to #7295
Currently a ClusterState update is issued for every index when deleting multiple indices, this causes timeout errors in most cases when deleting a large amount of indices. We changed the behavior so only one clusterState update (maximum two if some indices are locked ) is issued when deleting multiple indices. This makes large delete index operations much more faster and more fail-safe.