Deleting multiple indices can fail to report all failures #11189
Conversation
| for (final String index : concreteIndices) { | ||
| deleteIndexService.deleteIndex(new MetaDataDeleteIndexService.Request(index).timeout(request.timeout()).masterTimeout(request.masterNodeTimeout()), new MetaDataDeleteIndexService.Listener() { | ||
|
|
||
| private volatile Throwable lastFailure; | ||
| private volatile boolean ack = true; |
There was a problem hiding this comment.
can we move ack too together with lastFailure please?
|
Could we also have a better description for the PR? I don't understand what it is doing. thanks |
|
@javanna i've signed the CLA and updated the commit so |
|
@clintongormley i've updated the PR description. I hope it's more clear now. |
| @@ -83,22 +84,22 @@ protected void masterOperation(final DeleteIndexRequest request, final ClusterSt | |||
| } | |||
| // TODO: this API should be improved, currently, if one delete index failed, we send a failure, we should send a response array that includes all the indices that were deleted | |||
| final CountDown count = new CountDown(concreteIndices.length); | |||
| final AtomicReference<Throwable> lastFailure = new AtomicReference<>(); | |||
| final AtomicReference<Boolean> ack = new AtomicReference<>(true); | |||
There was a problem hiding this comment.
I think AtomicBoolean would be better instead of AtomicReference<Boolean>?
There was a problem hiding this comment.
ah sry, AtomicBoolean is better of course - updated
|
thanks @bogensberger I just left a small comment, thanks a lot for updating, I will look into writing a test for this and merge it. |
if the last delete operation succeeded
clintongormley
changed the title
clintongormley
added
the
:Data Management/Indices APIs
| @@ -83,22 +85,22 @@ protected void masterOperation(final DeleteIndexRequest request, final ClusterSt | |||
| } | |||
| // TODO: this API should be improved, currently, if one delete index failed, we send a failure, we should send a response array that includes all the indices that were deleted | |||
| final CountDown count = new CountDown(concreteIndices.length); | |||
| final AtomicReference<Throwable> lastFailure = new AtomicReference<>(); | |||
There was a problem hiding this comment.
Can we somehow make this a synchronized list and report all failures? I'd just add 2-N failures as suppressed to the first?
|
@javanna can you come back to this? I added a comment but you worked on it before so I wonder if you have something to add? |
|
I agree with your comment @s1monw . I'd love to have a simple test for it as well, I meant to come up with one but I never got to it :( |
|
is this fixed by #11258 ? |
|
great closing then, thanks again @bogensberger ! Superseded by #11258 . |
|
awesome! |
|
@javanna I labeled it fixed in 2.2 and 3.0 no? |
currently a delete is performed for every concreteIndex within the
DeleteIndexRequest.If an error occurs the exception is assigned to
lastFailureand after the last delete is performed the error is passed to the listener (if one did occur).The problem is that lastFailure is declared within the listeners passed to
deleteIndexService.deleteIndexand so only the failure of the last deleted index is considered and the errors of previous deleted indices are ignored. This patch fixes this behavior by declaringlastFailureoutside the loop.i had problems creating a proper test for this, maybe one of you can point me into the right direction or show me a similar test