Fix TestIndexWriterOnError.testIOError failure. #13436
Conversation
Pull request apache#13406 inadvertly broke Lucene's handling of tragic exceptions by stopping after the first `DocValuesProducer` whose `close()` calls throws an exception, instead of keeping calling `close()` on further producers in the list. This moves back to the previous behavior. Closes apache#13434
| IOUtils.applyToAll( | ||
| dvProducersGens.stream().mapToObj(Long::valueOf).toList(), | ||
| gen -> { | ||
| RefCount<DocValuesProducer> dvp = genDVProducers.get(gen); | ||
| assert dvp != null : "gen=" + gen; | ||
| dvp.decRef(); | ||
| }); |
There was a problem hiding this comment.
My only minor concern is that this reverts the minor improvement of using primitive values. But, it fixes a much bigger issue. I am fine with reverting only this part of the optimization that caused the bug.
There was a problem hiding this comment.
Yes, good catch. This fix is required to continue using IOUtils.
I don't know why I didn't read enough the method javadoc.
There was a problem hiding this comment.
Agreed @benwtrent. I optimized for making the code look as similar as before without reverting the move to LongArrayList, in order to minimize room for bugs given the upcoming release.
| IOUtils.applyToAll( | ||
| dvProducersGens.stream().mapToObj(Long::valueOf).toList(), | ||
| gen -> { | ||
| RefCount<DocValuesProducer> dvp = genDVProducers.get(gen); | ||
| assert dvp != null : "gen=" + gen; | ||
| dvp.decRef(); | ||
| }); |
There was a problem hiding this comment.
Yes, good catch. This fix is required to continue using IOUtils.
I don't know why I didn't read enough the method javadoc.
Pull request #13406 inadvertly broke Lucene's handling of tragic exceptions by stopping after the first
DocValuesProducerwhoseclose()calls throws an exception, instead of keeping callingclose()on further producers in the list.This moves back to the previous behavior.
Closes #13434