You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@lucene.apache.org by GitBox <gi...@apache.org> on 2021/09/08 05:57:53 UTC
[GitHub] [lucene-solr] zacharymorn opened a new pull request #2567: LUCENE-9662: CheckIndex should be concurrent (backporting)
zacharymorn opened a new pull request #2567:
URL: https://github.com/apache/lucene-solr/pull/2567
### Description
Backporting changes from the following commits:
1. "CheckIndex should be concurrent - parallelizing index check across segments": https://github.com/apache/lucene/commit/424192e1704664dc0ebc55109feaad5990b945cb
1. "fix test failure from merging away soft-deletes": https://github.com/apache/lucene/commit/34232430f200a0941de683d9035e08a4cbec9df4
Note: Commit "Update concurrent index checking usage instructions" in https://github.com/apache/lucene/pull/281 will also be added later once it was approved and merged.
### Merge conflict resolution
As the CheckIndex changes were added on top of a few changes only intended for 9.0 release / not backported to 8x, there were some merge conflicts in `CheckIndex` during the backporting and I have handled them as such:
1. Removed KNN vector checking code
2. Used `FutureArrays` instead of `Arrays` (the change was introduced in https://github.com/apache/lucene/commit/faaee86efb01fa6e431fcb129cfb956c7d62d514#diff-5f58f7980802006ec5dcfaa58cdfa6b218bfca46d8f317dcca3a1c5881b80c64)
--
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-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] zacharymorn commented on a change in pull request #2567: LUCENE-9662: CheckIndex should be concurrent (backporting)
Posted by GitBox <gi...@apache.org>.
zacharymorn commented on a change in pull request #2567:
URL: https://github.com/apache/lucene-solr/pull/2567#discussion_r705101495
##########
File path: lucene/core/src/java/org/apache/lucene/index/CheckIndex.java
##########
@@ -465,9 +493,22 @@ public void setChecksumsOnly(boolean v) {
private boolean checksumsOnly;
- /** Set infoStream where messages should go. If null, no
- * messages are printed. If verbose is true then more
- * details are printed. */
+ /** Set threadCount used for parallelizing index integrity checking. */
+ public void setThreadCount(int tc) {
+ if (tc <= 0) {
+ throw new IllegalArgumentException(
+ "setThreadCount requires a number larger than 0, but got: " + tc);
+ }
+ threadCount = tc;
+ }
+
+ // capped threadCount at 4 for default
+ private int threadCount = Math.min(Runtime.getRuntime().availableProcessors(), 4);
Review comment:
I have actually updated that in the other PR for updating instruction as well (since the updated instruction also mentioned about the number of CPU cores for default) https://github.com/apache/lucene/pull/281, and plan to add that commit into this PR once it is merged. Would you like me to track this separately instead?
--
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-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] mikemccand commented on pull request #2567: LUCENE-9662: CheckIndex should be concurrent (backporting)
Posted by GitBox <gi...@apache.org>.
mikemccand commented on pull request #2567:
URL: https://github.com/apache/lucene-solr/pull/2567#issuecomment-917040356
> Hi @mikemccand, I've added the usage instruction commit to this PR and merged. Thanks again for your review and approval!
Woot! Thank you @zacharymorn! What an exciting improvement :) Gives us a reason to make an eventual 8.11!
--
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-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] mikemccand commented on pull request #2567: LUCENE-9662: CheckIndex should be concurrent (backporting)
Posted by GitBox <gi...@apache.org>.
mikemccand commented on pull request #2567:
URL: https://github.com/apache/lucene-solr/pull/2567#issuecomment-916132838
Phew thanks @zacharymorn!
> I'll add that commit to this PR once it is approved and merged into main.
OK I just approved that one.
> 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
Aha! That's a great catch! So `CheckIndex` was failing to throw an exception if there was an index sort problem detected. Thank you for fixing this sneaky pre-existing bug!
--
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-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] zacharymorn commented on pull request #2567: LUCENE-9662: CheckIndex should be concurrent (backporting)
Posted by GitBox <gi...@apache.org>.
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] zacharymorn merged pull request #2567: LUCENE-9662: CheckIndex should be concurrent (backporting)
Posted by GitBox <gi...@apache.org>.
zacharymorn merged pull request #2567:
URL: https://github.com/apache/lucene-solr/pull/2567
--
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-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] mikemccand commented on pull request #2567: LUCENE-9662: CheckIndex should be concurrent (backporting)
Posted by GitBox <gi...@apache.org>.
mikemccand commented on pull request #2567:
URL: https://github.com/apache/lucene-solr/pull/2567#issuecomment-915557112
Did you also include backport for fixing `Usage: ...` output?
--
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-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] mikemccand commented on pull request #2567: LUCENE-9662: CheckIndex should be concurrent (backporting)
Posted by GitBox <gi...@apache.org>.
mikemccand commented on pull request #2567:
URL: https://github.com/apache/lucene-solr/pull/2567#issuecomment-915401627
Let's remember to also backport [fix for LUCENE-10092](https://issues.apache.org/jira/browse/LUCENE-10092).
--
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-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] zacharymorn commented on pull request #2567: LUCENE-9662: CheckIndex should be concurrent (backporting)
Posted by GitBox <gi...@apache.org>.
zacharymorn commented on pull request #2567:
URL: https://github.com/apache/lucene-solr/pull/2567#issuecomment-916674521
Hi @mikemccand, I've added the usage instruction commit to this PR and merged. Thanks again for your review and approval!
--
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-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] mikemccand commented on a change in pull request #2567: LUCENE-9662: CheckIndex should be concurrent (backporting)
Posted by GitBox <gi...@apache.org>.
mikemccand commented on a change in pull request #2567:
URL: https://github.com/apache/lucene-solr/pull/2567#discussion_r704763696
##########
File path: lucene/core/src/java/org/apache/lucene/index/CheckIndex.java
##########
@@ -465,9 +493,22 @@ public void setChecksumsOnly(boolean v) {
private boolean checksumsOnly;
- /** Set infoStream where messages should go. If null, no
- * messages are printed. If verbose is true then more
- * details are printed. */
+ /** Set threadCount used for parallelizing index integrity checking. */
+ public void setThreadCount(int tc) {
+ if (tc <= 0) {
+ throw new IllegalArgumentException(
+ "setThreadCount requires a number larger than 0, but got: " + tc);
+ }
+ threadCount = tc;
+ }
+
+ // capped threadCount at 4 for default
+ private int threadCount = Math.min(Runtime.getRuntime().availableProcessors(), 4);
Review comment:
Let's be sure to relax this cap (`4`) in a follow-on issue. I think for command-line invocation it's OK to use all cores by default.
--
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-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] zacharymorn commented on a change in pull request #2567: LUCENE-9662: CheckIndex should be concurrent (backporting)
Posted by GitBox <gi...@apache.org>.
zacharymorn commented on a change in pull request #2567:
URL: https://github.com/apache/lucene-solr/pull/2567#discussion_r704073179
##########
File path: lucene/backward-codecs/src/test/org/apache/lucene/index/TestBackwardsCompatibility.java
##########
@@ -1834,33 +1834,6 @@ private void searchExampleIndex(DirectoryReader reader) throws IOException {
assertTrue(firstDate <= lastDate);
}
- /**
- * Tests that {@link CheckIndex} can detect invalid sort on sorted indices created
- * before https://issues.apache.org/jira/browse/LUCENE-8592.
- */
- public void testSortedIndexWithInvalidSort() throws Exception {
Review comment:
This test now failed with the `CheckIndex` changes. Since it [was removed in `main` a few months ago](https://github.com/apache/lucene/commit/2afa6cb00a2933a15e65d714b3c564c314059466#diff-cb558a4f36adaa7a363b310a5ea0aa2afaf12b0177862ee36312007eeab4c08dL1729) but not backported, I temporarily removed it from here as well. However, I doubt this is the right way of handling this test failure, and suspect there might be a previous change in `CheckIndex` that was not compatible with this test and should be taken out as well. I'll look into this a bit more in the next few days.
--
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-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org