vigyasharma commented on code in PR #633:
URL: https://github.com/apache/lucene/pull/633#discussion_r862516293
##########
lucene/test-framework/src/java/org/apache/lucene/tests/index/MockRandomMergePolicy.java:
##########
@@ -86,6 +86,20 @@ public MergeSpecification findMerges(
return mergeSpec;
}
+ @Override
+ public MergeSpecification findMerges(CodecReader... readers) throws
IOException {
+ if (random.nextBoolean() == true) {
+ return super.findMerges(readers);
+ } else {
+ // create a oneMerge for each reader to let them get concurrently
processed by addIndexes()
Review Comment:
> Perhaps create specific tests for the ones that actually failed when this
was enabled for all tests?
> ...So there is definitely nothing wrong with those tests. Changes in them
should be reverted!
The only test that failed was `testAddIndicesWithSoftDeletes()`. It tests
two different scenarios of soft deletes. One with `addIndexes(readers)` where
it validates the no. of soft deletes after adding readers to an empty index.
The other is with `addIndexes(wrappedReaders)` where `wrappedReaders` filter
out the soft deletes, and so the resulting writer has `maxDoc ==
wrappedReader.numDocs()`.
The test was using `writer.cloneSegmentInfos().info(0)` to count number of
soft deletes in resulting index for scenario-1. This requires the index to be
empty and for only a single segment to get added. I believe new segments get
added at the end of the list. If the intent here is to ensure that only one
segment is added, maybe we should make it something like
`writer.cloneSegmentInfos().info(numSegments - 1)`? Or separately assert on the
no. of new segments added? It does seem outside the purpose of this test, which
is to check for soft delete retention.
I
[changed](https://github.com/apache/lucene/pull/633/files#diff-ce6b91c31190c4956eee8a734550af25047d33b92a71c3d7303eac4cc408c504R1796-R1800)
it to use a sum of softDeleteCount from every segment, which, per my
understanding, is also what
[`writer.getDocStats()`](https://github.com/apache/lucene/blob/main/lucene/core/src/java/org/apache/lucene/index/IndexWriter.java#L6192)
does. The test passes with both concurrent and single merge addIndexes, with
this change. Let me know if I should revert this to its former state.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org
