zacharymorn commented on pull request #2567: URL: https://github.com/apache/lucene-solr/pull/2567#issuecomment-915871720
> Let's remember to also backport [fix for LUCENE-10092](https://issues.apache.org/jira/browse/LUCENE-10092). Added to this PR. > Did you also include backport for fixing Usage: ... output? No I haven't yet, as the PR for that one is still pending for approval https://github.com/apache/lucene/pull/281. I'll add that commit to this PR once it is approved and merged into `main`. --- > It's curious that that one test case now fails -- that test is testing an edge case that Lucene 8.x allows, but 9.x will no longer allow. I would have thought CheckIndex should still pass in that test, i.e. this PR (adding concurrency) should not by itself have caused the break. Maybe something indeed broke in the backport, e.g. you accidentally carried over the stricter 9.x checking? Okay I figured out where the "issue" is. It turned out that in the old code prior to the concurrent changes, `segInfoStat.indexSortStatus.error` was not checked and handled like the rest of statuses: https://github.com/apache/lucene-solr/blob/7f876de69e83962572da5d1e859c4e03a6fe8556/lucene/core/src/java/org/apache/lucene/index/CheckIndex.java#L727-L748 So in the `TestBackwardsCompatibility#testSortedIndexWithInvalidSort` code, it was checking the index status to verify the sort result, without encountering any exception: https://github.com/apache/lucene-solr/blob/7f876de69e83962572da5d1e859c4e03a6fe8556/lucene/backward-codecs/src/test/org/apache/lucene/index/TestBackwardsCompatibility.java#L1856-L1860 When I worked on the concurrent changes, I noticed the `segInfoStat.indexSortStatus.error` handling discrepancy, and updated it to work like the rest of statuses check :D https://github.com/apache/lucene/blob/ee0695eda8bc45611cf0312ea3eb6a8819537537/lucene/core/src/java/org/apache/lucene/index/CheckIndex.java#L1053-L1056 and thus this exception throwing will eventually be translated into `RuntimeException` being thrown within the test, by this code in `TestUtil`: https://github.com/apache/lucene-solr/blob/7f876de69e83962572da5d1e859c4e03a6fe8556/lucene/test-framework/src/java/org/apache/lucene/util/TestUtil.java#L303-L312 (the previous one was essentially going through the `else` branch to not throw exception) Hence, I think the new behavior is expected, and updated the test accordingly in this commit https://github.com/apache/lucene-solr/pull/2567/commits/0e2d33e705ce3db0ef6be936943f106c9b608533 . -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
