Add estimatedByteSizes to merges kicked off by IndexWriter.addIndexes(CodecReader[])

[cwperks]

Description

This PR abstracts the logic to add estimatedByteSizes on merges to a method called addEstimatedBytesToMerge so that it can be re-used in multiple places.

This PR adds a simple test to ensure that estimated bytes are added to the merge after addEstimatedBytesToMerge(MergePolicy.OneMerge merge) is called

The test can be run with ./gradlew :lucene:core:test --tests TestIndexWriterMerging.testAddEstimatedBytesToMerge -i

Resolves #14993

[github-actions]

This PR does not have an entry in lucene/CHANGES.txt. Consider adding one. If the PR doesn't need a changelog entry, then add the skip-changelog label to it and you will stop receiving this reminder on future updates to the PR.

[msokolov]

so this moves the accounting from deep in registerMerge to the top? Did the test fail before this change?

[cwperks]

so this moves the accounting from deep in registerMerge to the top? Did the test fail before this change?

@msokolov merges registered in public void registerMerge(MergePolicy.OneMerge merge) did not get the estimated bytes added like those registered in private synchronized boolean registerMerge(MergePolicy.OneMerge merge) do. This leads to information missing in log statements like the one's pointed out in the issue description.

I added a new unit test for the new method testAddEstimatedBytesToMerge, but I think it may also be good to ensure merges get this information regardless of how they're registered in an integration test. Do you know the proper place to add a test like that?

[msokolov]

Lucene doesn't really separate unit tests from integration tests, because it doesn't ship a service. But you can write unit tests at different levels of abstraction. I think what you've done here (create an index, add some docs, trigger merges, check the output) is about what we'd expect. But if you want to also test the size estimation of merges kicked off by "internal" policies, maybe the best way is to use RandomIndexWriter, which will sometimes randomly kick off some merges of its own? TBH this isn't an area of the code I know super well, and I commented mostly to understand what was going on!

[dweiss]

A try-with-resources block on both, perhaps? This is important for clean up if a test fails.

[cwperks]

Changed to try-with-resources here.

Do you think we should create a mechanism to enforce this pattern repo wide? I see a number of tests cases (included ones in this suite) that do not follow the try-with-resources pattern.

[dweiss]

The project has 20+ years of history, this isn't surprising. I think we should fix these when we touch the code involved - one huge patch is probably not worth the effort here. New code should stick to new conventions (and utilities) available in the language.

[dweiss]

I would throw an UncheckedIOException here. If something is wrong, it'll be wrong. Let the caller know early.

[cwperks]

Changed to throw UncheckedIOException

[dweiss]

Can this method be static too? I wonder if it uses any IW's state at all.

[cwperks]

I tried to make it static, but I was not able to because it calls numDeletedDocs() which is non-static. When I tried to make numDeletedDocs() static I ran into other issues. I've been trying to contribute more to Lucene lately, but still very much coming up to speed on the repo so opted to start small. Do you know if numDeletedDocs() and any other methods it calls can be changed to static?

[dweiss]

Ok, let it stay as it is now then.