[test] Track index commits internally acquired by the commits listener in CombinedDeletionPolicy

[arteam]

After finishing an integration test, we run some checks against the test cluster, among others we assert there are no leaky acquired index commits left in InternalTestCluster#beforeIndexDeletion. The issue is that while we check the test cluster before we shut it down, we can't guarantee that there wouldn't be a new commit triggered by a background merge which will acquire an index commit. But we actually don't care about these commits acquired internally as part of CombinedDeletionPolicy#commitsListener callbacks. We just want to make sure that all index commits that have been acquired explicitly are also released. So, we make an explicit distinction between external and internal index commits that are tracked in CombinedDeletionPolicy.

ES-8407

[elasticsearchmachine]

Pinging @elastic/es-distributed (Team:Distributed)

[henningandersen]

This looks good to me though it is slightly controversial.

[fcofdez]

I would prefer to keep the acquiredIndexCommits map as it's today but maybe add an extra Set<IndexCommit> listenerAcquiredCommits or something like that that only gets populated with the commits acquired for the listener and we could simply remove them from that new set whenever the refCount reaches 0 in releaseCommit. That feels more correct to me.

[fcofdez]

In fact that was your original approach @arteam :D

[arteam]

@fcofdez Yes, it was! I'm happy to proceed with either approach. I'm going to wait for an additional opinion from @henningandersen or @ywangd .

[kingherc]

Looks good!

I also like @fcofdez 's comment on leaving acquiredIndexCommits as it was (with both internally/externally acquired commits), thus reducing risk for bugs in that crucial part of the code, and having a separate Set for externally acquired commits that will just be used for our test assertions (meaning any bug in the new code will only affect tests). Not saying there's a bug, but just feels safer.

[ywangd]

I left a question.

Personally I'd prefer to keep the production code changes as minimal as possible. So a separate set for tracking works better for me. I'd also suggest we wrap relevant logic with Assertions.ENABLED check when applicable.

[henningandersen]

Looks good and I am ok with reverting to the previous direction based on majority vote 🙂 .

[arteam]

@elasticmachine update branch

[ywangd]

LGTM

[arteam]

@elasticmachine update branch