LUCENE-15108 commit writer before opening DirectoryReader to fix test

[VivekKumarNeu]

Description

The test failed because DirectoryReader.open() tried to read the index while there were still uncommitted changes in the writer.

The fix is to call writer.commit() after we have made all the changes and before opening the reader. That makes sure the reader can actually see a proper index.

PS: This is my first commit. Please comment if I need to make any changes, and I will update the PR accordingly. Thank you.

[VivekKumarNeu]

@jainankitk @jpountz @rmuir - If you can take a look, thanks!

[msokolov]

Generally speaking it's not a problem to open a Reader on an index that has a writer open with pending changes. It's hard to understand what went wrong in the test from the description here so we can check whether this is an appropriate fix. Could you include the command you ran to reproduce the failed test (which this patch is fixing)? never mind - I found the description in the linked issue: I guess the index did not even exist yet. I wonder what would happen if you put the commit right after opening the index?

[VivekKumarNeu]

Hi @msokolov ,

You are correct. Due to the lack of an index, the test fails. I tried testing the scenario you mentioned, added writer.commit() under the try (DirectoryReader reader = maybeWrapWithMergingReader(DirectoryReader.open(dir))) { , but it fails with the same error. I also tried adding the writer.commit() just after creating the writer (RandomIndexWriter writer = new RandomIndexWriter(random(), dir, conf);), and that condition works and we do not see any test failure.

I believe this condition:
if (random().nextInt(31) == 0) { writer.commit(); }
will only work for a specific time. If this is not true, we are without an index, and this error occurs.

[vigyasharma]

Thanks for fixing this test, @VivekKumarNeu !

I believe this condition:
if (random().nextInt(31) == 0) { writer.commit(); }
will only work for a specific time. If this is not true, we are without an index, and this error occurs.

That seems to be the case. For the specific seed which fails, the test never does a commit so the index doesn't exist. It's interesting, the odds of this happening should be very low (~0.01%). After indexing every doc, there's a 1/31 probability that we commit. And we index over 256 docs! Anyway, I think your fix of doing one final commit before we read this index makes sense.

Your actual fix is just a one line change, but this PR shows a lot of diffs from main. Can you fix that before we approve and merge?

[msfroh]

I tried testing the scenario you mentioned, added writer.commit() under the try (DirectoryReader reader = maybeWrapWithMergingReader(DirectoryReader.open(dir))) { , but it fails with the same error.

I think Mike's suggestion was to add a writer.commit() call right after the RandomIndexWriter writer = new RandomIndexWriter(random(), dir, conf); line. That is -- write an empty commit as soon as the writer is opened. @msokolov, was that what you had in mind?

I also tried adding the writer.commit() just after creating the writer (RandomIndexWriter writer = new RandomIndexWriter(random(), dir, conf);), and that condition works and we do not see any test failure.

So, that would fix it. It essentially replaces the "no commit written" case with "empty commit written".

[msokolov]

Yes, @msfroh, exactly -- I was concerned that the commit logic below was actually part of the test (maybe?) and was thinking to perturb it as little as possible. The test as it was would also test the case where there are some uncommitted updates that would be flushed when the reader is opened (I think), but if we add the explicit commit after indexing all the docs then this case will no longer be tested.

[msfroh]

The test as it was would also test the case where there are some uncommitted updates that would be flushed when the reader is opened (I think)

In this case, the reader isn't being opened from the writer, but rather gets opened against the directory. So, the pending changes wouldn't get flushed AFAIK. (So the deletes don't get applied until the forceMerge, which triggers a flush + commit.)

It's worth noting that doTestSortedSetVsStoredFields and doTestSortedVsStoredFields do open the reader from the writer, and are otherwise similar in structure to this method. So, I think the most consistent thing is to do the same in doTestSortedNumericsVsStoredFields.

@VivekKumarNeu -- can you please confirm that replacing the DirectoryReader.open lines with writer.getReader() fixes the issue as well?

[vigyasharma]

Yes, @msfroh, exactly -- I was concerned that the commit logic below was actually part of the test (maybe?) and was thinking to perturb it as little as possible.

Ah right, that makes sense!

[VivekKumarNeu]

Thanks so much for your input @msokolov ! It totally makes sense to keep the original test intent and avoid adding that writer.commit().

@msfroh , I updated the test to use writer.getReader() instead of DirectoryReader.open(dir), which makes it match the structure of our other tests. The test is happy now! Thanks for looking into it.

Updated the PR.
cc: @vigyasharma

[VivekKumarNeu]

@vigyasharma Could you please review/merge the PR when you have time? Thank you!