You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@lucene.apache.org by "slow-J (via GitHub)" <gi...@apache.org> on 2023/11/11 15:33:51 UTC
[PR] CheckIndex - Making -fast the default behaviour [lucene]
slow-J opened a new pull request, #12797:
URL: https://github.com/apache/lucene/pull/12797
Closes #11023
I found this old issue and reached out to Zach @zacharymorn, who said that he doesn't plan on picking it up currently so I had a go at tackling this.
CheckIndex - Making -fast the default behaviour
1. Making -fast the new default.
2. The previous -slow is moved to -slower
3. The previous default behavior (checksum + segment file content) is activated by -slow.
### Test
Command executed: `java -server -Xms2g -Xmx2g --add-modules jdk.incubator.vector -XX:+HeapDumpOnOutOfMemoryError -XX:+UseParallelGC -classpath ./lucene/core/build/libs/lucene-core-10.0.0-SNAPSHOT.jar -ea org.apache.lucene.index.CheckIndex -threadCount 16 myindex_dir/index`
Default: `Took 1.379 sec total.`
Slow: `Took 74.824 sec total.`
Slower: `Took 136.988 sec total.`
--
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
Re: [PR] CheckIndex - Adding a `-level` parameter to give ability to control index check detail programmatically [lucene]
Posted by "slow-J (via GitHub)" <gi...@apache.org>.
slow-J commented on PR #12797:
URL: https://github.com/apache/lucene/pull/12797#issuecomment-1829926883
Hi @mikemccand thanks for all the comments, addressed them all and now resolved the new merge conflicts!
--
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
Re: [PR] CheckIndex - Adding a `-level` parameter to give ability to control index check detail programmatically [lucene]
Posted by "slow-J (via GitHub)" <gi...@apache.org>.
slow-J commented on code in PR #12797:
URL: https://github.com/apache/lucene/pull/12797#discussion_r1400999774
##########
lucene/core/src/java/org/apache/lucene/index/CheckIndex.java:
##########
@@ -2479,15 +2462,11 @@ public static Status.TermIndexStatus testPostings(CodecReader reader, PrintStrea
* @lucene.experimental
*/
public static Status.TermIndexStatus testPostings(
- CodecReader reader,
- PrintStream infoStream,
- boolean verbose,
- boolean doSlowChecks,
- boolean failFast)
+ CodecReader reader, PrintStream infoStream, boolean verbose, int level, boolean failFast)
throws IOException {
- // TODO: we should go and verify term vectors match, if
- // doSlowChecks is on...
+ // TODO: we should go and verify term vectors match, if the Level is high enough to
+ // include slow checks
Review Comment:
Maybe the `EXCEPTIONALLY_SLOW_CHECKS` could be the level 4...
Will leave as is for now.
--
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
Re: [PR] CheckIndex - Adding a `-detailLevel` parameter to give ability to control index check detail programmatically [lucene]
Posted by "slow-J (via GitHub)" <gi...@apache.org>.
slow-J commented on PR #12797:
URL: https://github.com/apache/lucene/pull/12797#issuecomment-1819907043
> I like the change. At first sight, `detailLevel` sounded to me like a verbosity level, ie. something that affects how much is printed rather than how much work is done. What about calling that option just `level`?
Thanks Adrien, naming is hard :D I was thinking `detailLevel` or `checkLevel`, but I can change to just `level`.
--
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
Re: [PR] CheckIndex - Adding a `-level` parameter to give ability to control index check detail programmatically [lucene]
Posted by "slow-J (via GitHub)" <gi...@apache.org>.
slow-J commented on code in PR #12797:
URL: https://github.com/apache/lucene/pull/12797#discussion_r1401195063
##########
lucene/core/src/java/org/apache/lucene/index/CheckIndex.java:
##########
@@ -4127,15 +4123,33 @@ public static Options parseOptions(String[] args) {
int i = 0;
while (i < args.length) {
String arg = args[i];
- if ("-fast".equals(arg)) {
- opts.doChecksumsOnly = true;
+ if ("-level".equals(arg)) {
+ if (i == args.length - 1) {
+ throw new IllegalArgumentException("ERROR: missing value for -level option");
+ }
+ i++;
+ int level = Integer.parseInt(args[i]);
+ if (level < Level.MIN_VALUE || level > Level.MAX_VALUE) {
+ throw new IllegalArgumentException(
+ String.format(
+ "ERROR: value for -level option is out of bounds. Please use a value "
+ + "from '%d'->'%d'",
+ Level.MIN_VALUE, Level.MAX_VALUE));
Review Comment:
Ah I see what you meant earlier now, will add.
--
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
Re: [PR] CheckIndex - Making -fast the default behaviour [lucene]
Posted by "rmuir (via GitHub)" <gi...@apache.org>.
rmuir commented on code in PR #12797:
URL: https://github.com/apache/lucene/pull/12797#discussion_r1390277473
##########
lucene/core/src/java/org/apache/lucene/index/CheckIndex.java:
##########
@@ -3974,7 +3974,7 @@ public static class Options {
boolean doExorcise = false;
boolean doSlowChecks = false;
boolean verbose = false;
- boolean doChecksumsOnly = false;
+ boolean doChecksumsOnly = true;
Review Comment:
I think we have to fix the CheckIndex java API as well. Test logic such as `/lucene/test-framework/src/java/org/apache/lucene/tests/util/TestUtil.java` should always be using `-slow` or `-slower` since we want to find bugs in codec logic.
--
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
Re: [PR] CheckIndex - Making -fast the default behaviour [lucene]
Posted by "slow-J (via GitHub)" <gi...@apache.org>.
slow-J commented on code in PR #12797:
URL: https://github.com/apache/lucene/pull/12797#discussion_r1391501828
##########
lucene/core/src/java/org/apache/lucene/index/CheckIndex.java:
##########
@@ -3974,7 +3974,7 @@ public static class Options {
boolean doExorcise = false;
boolean doSlowChecks = false;
boolean verbose = false;
- boolean doChecksumsOnly = false;
+ boolean doChecksumsOnly = true;
Review Comment:
Thanks for the suggestions Robert!
I should have some time in the week to implement them.
--
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
Re: [PR] CheckIndex - Adding a `-level` parameter to give ability to control index check detail programmatically [lucene]
Posted by "slow-J (via GitHub)" <gi...@apache.org>.
slow-J commented on code in PR #12797:
URL: https://github.com/apache/lucene/pull/12797#discussion_r1401175756
##########
lucene/core/src/java/org/apache/lucene/index/CheckIndex.java:
##########
@@ -3661,7 +3640,7 @@ private static void checkDocValues(
*/
public static Status.TermVectorStatus testTermVectors(CodecReader reader, PrintStream infoStream)
throws IOException {
- return testTermVectors(reader, infoStream, false, false, false);
+ return testTermVectors(reader, infoStream, false, Level.MIN_LEVEL_FOR_SLOW_CHECKS, false);
Review Comment:
Good find, this was unintentional, will revert.
Although, this method doesn't seem to be used?
Maybe this method should be removed in a follow-up?
--
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
Re: [PR] CheckIndex - Adding a `-level` parameter to give ability to control index check detail programmatically [lucene]
Posted by "slow-J (via GitHub)" <gi...@apache.org>.
slow-J commented on PR #12797:
URL: https://github.com/apache/lucene/pull/12797#issuecomment-1830400036
> Thanks @slow-J -- looks great!
>
> This is a 10.0 only change right? I'll merge soon.
Thanks!
And yes 10.0 only.
--
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
Re: [PR] CheckIndex - Adding a `-detailLevel` parameter to give ability to control index check detail programmatically [lucene]
Posted by "slow-J (via GitHub)" <gi...@apache.org>.
slow-J commented on code in PR #12797:
URL: https://github.com/apache/lucene/pull/12797#discussion_r1399736567
##########
lucene/core/src/java/org/apache/lucene/index/CheckIndex.java:
##########
@@ -4051,15 +4057,30 @@ public static Options parseOptions(String[] args) {
int i = 0;
while (i < args.length) {
String arg = args[i];
- if ("-fast".equals(arg)) {
- opts.doChecksumsOnly = true;
+ if ("-detailLevel".equals(arg)) {
+ if (i == args.length - 1) {
+ throw new IllegalArgumentException("ERROR: missing value for -detailLevel option");
+ }
+ i++;
+ int detailLevel = Integer.parseInt(args[i]);
+ if (detailLevel < DetailLevel.MIN_VALUE || detailLevel > DetailLevel.MAX_VALUE) {
+ throw new IllegalArgumentException(
+ String.format(
+ "ERROR: value for -detailLevel option is out of bounds. Please use a value "
+ + "from '%d'->'%d'",
+ DetailLevel.MIN_VALUE, DetailLevel.MAX_VALUE));
+ }
+ opts.detailLevel = detailLevel;
+ } else if ("-fast".equals(arg)) {
+ System.err.println("-fast is deprecated, verifying file checksums only is now the default");
+ } else if ("-slow".equals(arg)) {
+ System.err.println("-slow is deprecated, use '-detailLevel 3' instead for slow checks");
+ opts.detailLevel = DetailLevel.MIN_LEVEL_FOR_SLOW_CHECKS;
} else if ("-exorcise".equals(arg)) {
opts.doExorcise = true;
} else if ("-crossCheckTermVectors".equals(arg)) {
- System.err.println("-crossCheckTermVectors is deprecated, use -slow instead");
- opts.doSlowChecks = true;
- } else if ("-slow".equals(arg)) {
- opts.doSlowChecks = true;
+ System.err.println("-crossCheckTermVectors is deprecated, use '-detailLevel 3' instead");
Review Comment:
Thanks @rmuir, will add an entry to `MIGRATE.md` and will leave the deprecated params for now, setting for removal in Lucene 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
Re: [PR] CheckIndex - Adding a `-level` parameter to give ability to control index check detail programmatically [lucene]
Posted by "mikemccand (via GitHub)" <gi...@apache.org>.
mikemccand merged PR #12797:
URL: https://github.com/apache/lucene/pull/12797
--
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
Re: [PR] CheckIndex - Adding a `-level` parameter to give ability to control index check detail programmatically [lucene]
Posted by "slow-J (via GitHub)" <gi...@apache.org>.
slow-J commented on code in PR #12797:
URL: https://github.com/apache/lucene/pull/12797#discussion_r1401212917
##########
lucene/core/src/java/org/apache/lucene/index/CheckIndex.java:
##########
@@ -4191,7 +4207,8 @@ public static Options parseOptions(String[] args) {
+ "If no package is specified the "
+ FSDirectory.class.getPackage().getName()
+ " package will be used.\n"
- + "\n"
+ + "CheckIndex only verifies file checksums as default.\n"
+ + "Use -level with value of '2' or higher if you also want to check segment file contents.\n\n"
Review Comment:
For level 3, besides the `cross verify postings/TVs` I see also `Checking score blocks`.
--
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
Re: [PR] CheckIndex - Adding a `-detailLevel` parameter to give ability to control index check detail programmatically [lucene]
Posted by "slow-J (via GitHub)" <gi...@apache.org>.
slow-J commented on PR #12797:
URL: https://github.com/apache/lucene/pull/12797#issuecomment-1819836777
> This looks fantastic! I left a minor comment, I think a mention in `MIGRATE.md` is valuable for this change.
>
> It would be good to get another reviewer to make sure I didn't miss something as checkindex is really critical.
Thanks for the review and approval @rmuir! Added the MIGRATE.md note there.
I think @mikemccand was looking into `CheckIndex` recently, so maybe he may be interested in this PR?
--
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
Re: [PR] CheckIndex - Making -fast the default behaviour [lucene]
Posted by "slow-J (via GitHub)" <gi...@apache.org>.
slow-J commented on code in PR #12797:
URL: https://github.com/apache/lucene/pull/12797#discussion_r1393046003
##########
lucene/core/src/java/org/apache/lucene/index/CheckIndex.java:
##########
@@ -3974,7 +3974,7 @@ public static class Options {
boolean doExorcise = false;
boolean doSlowChecks = false;
boolean verbose = false;
- boolean doChecksumsOnly = false;
+ boolean doChecksumsOnly = true;
Review Comment:
I like eliminating the boolean, will instead use a `-detailLevel` param. I'll make sure that the unit tests are doing the integrity checks like before.
--
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
Re: [PR] CheckIndex - Making -fast the default behaviour [lucene]
Posted by "slow-J (via GitHub)" <gi...@apache.org>.
slow-J commented on code in PR #12797:
URL: https://github.com/apache/lucene/pull/12797#discussion_r1393046003
##########
lucene/core/src/java/org/apache/lucene/index/CheckIndex.java:
##########
@@ -3974,7 +3974,7 @@ public static class Options {
boolean doExorcise = false;
boolean doSlowChecks = false;
boolean verbose = false;
- boolean doChecksumsOnly = false;
+ boolean doChecksumsOnly = true;
Review Comment:
I like eliminating the boolean, will instead use a `-detailLevel` param.
--
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
Re: [PR] CheckIndex - Making -fast the default behaviour [lucene]
Posted by "rmuir (via GitHub)" <gi...@apache.org>.
rmuir commented on code in PR #12797:
URL: https://github.com/apache/lucene/pull/12797#discussion_r1390277688
##########
lucene/core/src/java/org/apache/lucene/index/CheckIndex.java:
##########
@@ -3974,7 +3974,7 @@ public static class Options {
boolean doExorcise = false;
boolean doSlowChecks = false;
boolean verbose = false;
- boolean doChecksumsOnly = false;
+ boolean doChecksumsOnly = true;
Review Comment:
and feel free to think of alternative way to integrate this other than `-slow`, `-slower`, etc. It is a breaking change, major release is a good time to do it IMO, but maybe it could be done via numeric level (like verbosity in other commandline tools) or some other way? It would eliminate the booleans too which aren't great. And I think we should take checkindex API in to consideration for this change as well, for anyone calling it programmatically.
--
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
Re: [PR] CheckIndex - Making -fast the default behaviour [lucene]
Posted by "slow-J (via GitHub)" <gi...@apache.org>.
slow-J commented on code in PR #12797:
URL: https://github.com/apache/lucene/pull/12797#discussion_r1393115663
##########
lucene/core/src/java/org/apache/lucene/index/CheckIndex.java:
##########
@@ -4051,15 +4057,30 @@ public static Options parseOptions(String[] args) {
int i = 0;
while (i < args.length) {
String arg = args[i];
- if ("-fast".equals(arg)) {
- opts.doChecksumsOnly = true;
+ if ("-detailLevel".equals(arg)) {
+ if (i == args.length - 1) {
+ throw new IllegalArgumentException("ERROR: missing value for -detailLevel option");
+ }
+ i++;
+ int detailLevel = Integer.parseInt(args[i]);
+ if (detailLevel < DetailLevel.MIN_VALUE || detailLevel > DetailLevel.MAX_VALUE) {
+ throw new IllegalArgumentException(
+ String.format(
+ "ERROR: value for -detailLevel option is out of bounds. Please use a value "
+ + "from '%d'->'%d'",
+ DetailLevel.MIN_VALUE, DetailLevel.MAX_VALUE));
+ }
+ opts.detailLevel = detailLevel;
+ } else if ("-fast".equals(arg)) {
+ System.err.println("-fast is deprecated, verifying file checksums only is now the default");
+ } else if ("-slow".equals(arg)) {
+ System.err.println("-slow is deprecated, use '-detailLevel 3' instead for slow checks");
+ opts.detailLevel = DetailLevel.MIN_LEVEL_FOR_SLOW_CHECKS;
} else if ("-exorcise".equals(arg)) {
opts.doExorcise = true;
} else if ("-crossCheckTermVectors".equals(arg)) {
- System.err.println("-crossCheckTermVectors is deprecated, use -slow instead");
- opts.doSlowChecks = true;
- } else if ("-slow".equals(arg)) {
- opts.doSlowChecks = true;
+ System.err.println("-crossCheckTermVectors is deprecated, use '-detailLevel 3' instead");
Review Comment:
Should we remove the deprecated parameters (crossCheckTermVectors, fast, slow)?
--
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
Re: [PR] CheckIndex - Making -fast the default behaviour [lucene]
Posted by "rmuir (via GitHub)" <gi...@apache.org>.
rmuir commented on PR #12797:
URL: https://github.com/apache/lucene/pull/12797#issuecomment-1806908329
Maybe another way to help the user with this change is to inform them when checks are being skipped.
e.g. instead of:
```java
if (checksumsOnly == false) {
// Test Livedocs ...
```
We could do something like:
```java
if (checksumsOnly) {
msg(infoStream, "Skipping logical integrity checks: pass -ea -slow to check logical integrity")
} else {
// Test Livedocs ...
```
https://github.com/apache/lucene/blob/main/lucene/core/src/java/org/apache/lucene/index/CheckIndex.java#L994
--
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
Re: [PR] CheckIndex - Adding a `-level` parameter to give ability to control index check detail programmatically [lucene]
Posted by "slow-J (via GitHub)" <gi...@apache.org>.
slow-J commented on code in PR #12797:
URL: https://github.com/apache/lucene/pull/12797#discussion_r1400997709
##########
lucene/MIGRATE.md:
##########
@@ -101,6 +101,13 @@ The deprecated getter for the `Executor` that was optionally provided to the `In
has been removed. Users that want to execute concurrent tasks should rely instead on the `TaskExecutor`
that the searcher holds, retrieved via `IndexSearcher#getTaskExecutor`.
+### CheckIndex params -slow and -fast are deprecated, replaced by -level X (GITHUB#11023)
+
+The `CheckIndex` former `-fast` behaviour of performing checksum checks only, is now the default.
+Added a new parameter: `-level X`, to set the detail level of the index check. The higher the value, the more checks are performed.
+Sample `-level` usage: `1` (Default) - Checksum checks only, `2` - all level 1 checks as well as logical integrity checks, `3` - all
+level 2 checks as well as slow checks.
Review Comment:
Yes, they are inclusive. There is no 4 currently.
##########
lucene/core/src/java/org/apache/lucene/index/CheckIndex.java:
##########
@@ -442,19 +442,19 @@ public void close() throws IOException {
IOUtils.close(writeLock);
}
- private boolean doSlowChecks;
+ private int level;
/**
- * If true, additional slow checks are performed. This will likely drastically increase time it
- * takes to run CheckIndex!
+ * Sets Level, the higher the value, the more additional checks are performed. This will likely
+ * drastically increase time it takes to run CheckIndex! See {@link Level}
*/
- public void setDoSlowChecks(boolean v) {
- doSlowChecks = v;
+ public void setLevel(int v) {
+ level = v;
Review Comment:
We do `throw IllegalArgumentException` if the input is below or above the limit :)
--
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
Re: [PR] CheckIndex - Adding a `-detailLevel` parameter to give ability to control index check detail programmatically [lucene]
Posted by "rmuir (via GitHub)" <gi...@apache.org>.
rmuir commented on code in PR #12797:
URL: https://github.com/apache/lucene/pull/12797#discussion_r1399324212
##########
lucene/core/src/java/org/apache/lucene/index/CheckIndex.java:
##########
@@ -4051,15 +4057,30 @@ public static Options parseOptions(String[] args) {
int i = 0;
while (i < args.length) {
String arg = args[i];
- if ("-fast".equals(arg)) {
- opts.doChecksumsOnly = true;
+ if ("-detailLevel".equals(arg)) {
+ if (i == args.length - 1) {
+ throw new IllegalArgumentException("ERROR: missing value for -detailLevel option");
+ }
+ i++;
+ int detailLevel = Integer.parseInt(args[i]);
+ if (detailLevel < DetailLevel.MIN_VALUE || detailLevel > DetailLevel.MAX_VALUE) {
+ throw new IllegalArgumentException(
+ String.format(
+ "ERROR: value for -detailLevel option is out of bounds. Please use a value "
+ + "from '%d'->'%d'",
+ DetailLevel.MIN_VALUE, DetailLevel.MAX_VALUE));
+ }
+ opts.detailLevel = detailLevel;
+ } else if ("-fast".equals(arg)) {
+ System.err.println("-fast is deprecated, verifying file checksums only is now the default");
+ } else if ("-slow".equals(arg)) {
+ System.err.println("-slow is deprecated, use '-detailLevel 3' instead for slow checks");
+ opts.detailLevel = DetailLevel.MIN_LEVEL_FOR_SLOW_CHECKS;
} else if ("-exorcise".equals(arg)) {
opts.doExorcise = true;
} else if ("-crossCheckTermVectors".equals(arg)) {
- System.err.println("-crossCheckTermVectors is deprecated, use -slow instead");
- opts.doSlowChecks = true;
- } else if ("-slow".equals(arg)) {
- opts.doSlowChecks = true;
+ System.err.println("-crossCheckTermVectors is deprecated, use '-detailLevel 3' instead");
Review Comment:
I think we should. personally i think it is good enough to give checkindex a clean api (based on detail level) for the major release and add notes to `MIGRATE.md` as appropriate?
--
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
Re: [PR] CheckIndex - Adding a `-level` parameter to give ability to control index check detail programmatically [lucene]
Posted by "jpountz (via GitHub)" <gi...@apache.org>.
jpountz commented on code in PR #12797:
URL: https://github.com/apache/lucene/pull/12797#discussion_r1401280939
##########
lucene/core/src/java/org/apache/lucene/index/CheckIndex.java:
##########
@@ -2382,7 +2365,7 @@ private static Status.TermIndexStatus checkFields(
static void checkImpacts(Impacts impacts, int lastTarget) {
final int numLevels = impacts.numLevels();
if (numLevels < 1) {
- throw new CheckIndexException("The number of levels must be >= 1, got " + numLevels);
+ throw new CheckIndexException("The number of impactsLevels must be >= 1, got " + numLevels);
Review Comment:
```suggestion
throw new CheckIndexException("The number of impact levels must be >= 1, got " + numLevels);
```
--
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
Re: [PR] CheckIndex - Making -fast the default behaviour [lucene]
Posted by "slow-J (via GitHub)" <gi...@apache.org>.
slow-J commented on PR #12797:
URL: https://github.com/apache/lucene/pull/12797#issuecomment-1810872291
> ```java
> msg(infoStream, "Skipping logical integrity checks: pass -ea -slow to check logical integrity")
> ```
I tried and this is not possible to do, because while running the tool the output is spammed with an unusable amount (>10,000) of "Skipping" messages.
--
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
Re: [PR] CheckIndex - Adding a `-detailLevel` parameter to give ability to control index check detail programmatically [lucene]
Posted by "slow-J (via GitHub)" <gi...@apache.org>.
slow-J commented on PR #12797:
URL: https://github.com/apache/lucene/pull/12797#issuecomment-1819814215
Git merge error.... Let me try fix.
--
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
Re: [PR] CheckIndex - Adding a `-level` parameter to give ability to control index check detail programmatically [lucene]
Posted by "mikemccand (via GitHub)" <gi...@apache.org>.
mikemccand commented on code in PR #12797:
URL: https://github.com/apache/lucene/pull/12797#discussion_r1400425827
##########
lucene/core/src/java/org/apache/lucene/index/CheckIndex.java:
##########
@@ -442,19 +442,19 @@ public void close() throws IOException {
IOUtils.close(writeLock);
}
- private boolean doSlowChecks;
+ private int level;
/**
- * If true, additional slow checks are performed. This will likely drastically increase time it
- * takes to run CheckIndex!
+ * Sets Level, the higher the value, the more additional checks are performed. This will likely
+ * drastically increase time it takes to run CheckIndex! See {@link Level}
*/
- public void setDoSlowChecks(boolean v) {
- doSlowChecks = v;
+ public void setLevel(int v) {
+ level = v;
Review Comment:
Hmm could we `throw IllegalArgumentException` if the limit is too low or high? I get annoyed with tools that tell me "just add another `-v` for more verbosity" but don't tell me when I can/should stop :)
##########
lucene/MIGRATE.md:
##########
@@ -101,6 +101,13 @@ The deprecated getter for the `Executor` that was optionally provided to the `In
has been removed. Users that want to execute concurrent tasks should rely instead on the `TaskExecutor`
that the searcher holds, retrieved via `IndexSearcher#getTaskExecutor`.
+### CheckIndex params -slow and -fast are deprecated, replaced by -level X (GITHUB#11023)
+
+The `CheckIndex` former `-fast` behaviour of performing checksum checks only, is now the default.
+Added a new parameter: `-level X`, to set the detail level of the index check. The higher the value, the more checks are performed.
+Sample `-level` usage: `1` (Default) - Checksum checks only, `2` - all level 1 checks as well as logical integrity checks, `3` - all
+level 2 checks as well as slow checks.
Review Comment:
Are valid levels 1 .. 3 inclusive? Does it go to 4?
##########
lucene/core/src/java/org/apache/lucene/index/CheckIndex.java:
##########
@@ -3744,7 +3718,7 @@ public static Status.TermVectorStatus testTermVectors(
+ " but FieldInfo has storeTermVector=false");
}
- if (doSlowChecks) {
+ if (level >= Level.MIN_LEVEL_FOR_SLOW_CHECKS) {
Terms terms = tfv.terms(field);
Review Comment:
Oh I see -- we are already doing the cross posting/TVs check for `MIN_LEVEL_FOR_SLOW_CHECKS`. I think we can remove the above `TODO` now? You did it!
Edit: hmm I'm confused -- were we already cross-checking when `doSlowChecks = true` before? Is this the only thing enabled by slow checks?
##########
lucene/core/src/java/org/apache/lucene/index/CheckIndex.java:
##########
@@ -4127,15 +4123,33 @@ public static Options parseOptions(String[] args) {
int i = 0;
while (i < args.length) {
String arg = args[i];
- if ("-fast".equals(arg)) {
- opts.doChecksumsOnly = true;
+ if ("-level".equals(arg)) {
+ if (i == args.length - 1) {
+ throw new IllegalArgumentException("ERROR: missing value for -level option");
+ }
+ i++;
+ int level = Integer.parseInt(args[i]);
+ if (level < Level.MIN_VALUE || level > Level.MAX_VALUE) {
+ throw new IllegalArgumentException(
+ String.format(
+ "ERROR: value for -level option is out of bounds. Please use a value "
+ + "from '%d'->'%d'",
+ Level.MIN_VALUE, Level.MAX_VALUE));
+ }
+ opts.level = level;
+ } else if ("-fast".equals(arg)) {
+ // Deprecated. Remove in Lucene 11.
+ System.err.println("-fast is deprecated, verifying file checksums only is now the default");
Review Comment:
For full disambiguation, could you add `-level 0` into the message making it clear that is the equivalent if what `-fast` used to be and is now the default?
##########
lucene/core/src/java/org/apache/lucene/index/CheckIndex.java:
##########
@@ -4213,10 +4230,6 @@ public static Options parseOptions(String[] args) {
throw new IllegalArgumentException("ERROR: cannot specify both -exorcise and -segment");
}
- if (opts.doChecksumsOnly && opts.doSlowChecks) {
- throw new IllegalArgumentException("ERROR: cannot specify both -fast and -slow");
Review Comment:
Nice -- a level is so much better!
##########
lucene/core/src/java/org/apache/lucene/index/CheckIndex.java:
##########
@@ -4127,15 +4123,33 @@ public static Options parseOptions(String[] args) {
int i = 0;
while (i < args.length) {
String arg = args[i];
- if ("-fast".equals(arg)) {
- opts.doChecksumsOnly = true;
+ if ("-level".equals(arg)) {
+ if (i == args.length - 1) {
+ throw new IllegalArgumentException("ERROR: missing value for -level option");
+ }
+ i++;
+ int level = Integer.parseInt(args[i]);
+ if (level < Level.MIN_VALUE || level > Level.MAX_VALUE) {
+ throw new IllegalArgumentException(
+ String.format(
+ "ERROR: value for -level option is out of bounds. Please use a value "
+ + "from '%d'->'%d'",
+ Level.MIN_VALUE, Level.MAX_VALUE));
Review Comment:
Awesome, thanks for catching this entry point (command-line). I think we should catch invalid API calls too?
##########
lucene/core/src/java/org/apache/lucene/index/CheckIndex.java:
##########
@@ -3661,7 +3640,7 @@ private static void checkDocValues(
*/
public static Status.TermVectorStatus testTermVectors(CodecReader reader, PrintStream infoStream)
throws IOException {
- return testTermVectors(reader, infoStream, false, false, false);
+ return testTermVectors(reader, infoStream, false, Level.MIN_LEVEL_FOR_SLOW_CHECKS, false);
Review Comment:
Hmm did we intentionally change the slowness here? Previously it was fast (`doSlowChecks = false`) but now it's slow?
##########
lucene/core/src/java/org/apache/lucene/index/CheckIndex.java:
##########
@@ -2073,9 +2056,9 @@ private static Status.TermIndexStatus checkFields(
Impacts impacts = impactsEnum.getImpacts();
checkImpacts(impacts, doc);
maxFreq = Integer.MAX_VALUE;
- for (int level = 0; level < impacts.numLevels(); ++level) {
- if (impacts.getDocIdUpTo(level) >= max) {
- List<Impact> perLevelImpacts = impacts.getImpacts(level);
+ for (int impactsLevel = 0; impactsLevel < impacts.numLevels(); ++impactsLevel) {
Review Comment:
Heh name collision! Naming is the hardest part of our jobs...
##########
lucene/core/src/java/org/apache/lucene/index/CheckIndex.java:
##########
@@ -4191,7 +4207,8 @@ public static Options parseOptions(String[] args) {
+ "If no package is specified the "
+ FSDirectory.class.getPackage().getName()
+ " package will be used.\n"
- + "\n"
+ + "CheckIndex only verifies file checksums as default.\n"
+ + "Use -level with value of '2' or higher if you also want to check segment file contents.\n\n"
Review Comment:
Hmm should this be `1 or higher`? 0 == fast, 1 == some contents checked, 2 == some more contents checked (hmm what's different here?), 3 == insanely slow (cross verify postings/TVs)?
Edit OK I see -- `1` is the min level, so `1 == fast (validate checksums)`, `2 == checkIntegrity` (hmm not sure what this does beyond checksums), `3 == insanely slow (cross verify postings/TVs)` -- are there any other checks enabled at level 3 besides the cross verification?
##########
lucene/core/src/java/org/apache/lucene/index/CheckIndex.java:
##########
@@ -2479,15 +2462,11 @@ public static Status.TermIndexStatus testPostings(CodecReader reader, PrintStrea
* @lucene.experimental
*/
public static Status.TermIndexStatus testPostings(
- CodecReader reader,
- PrintStream infoStream,
- boolean verbose,
- boolean doSlowChecks,
- boolean failFast)
+ CodecReader reader, PrintStream infoStream, boolean verbose, int level, boolean failFast)
throws IOException {
- // TODO: we should go and verify term vectors match, if
- // doSlowChecks is on...
+ // TODO: we should go and verify term vectors match, if the Level is high enough to
+ // include slow checks
Review Comment:
Maybe we need a new `MIN_LEVEL_FOR_EXCEPTIONALLY_SLOW_CHECKS`! Cross-verification of postings and term vectors is indeed heinously slow...
But let's not do this in this PR -- this is already an awesome change -- PnP (progress not perfection, not plug and play).
##########
lucene/core/src/java/org/apache/lucene/index/CheckIndex.java:
##########
@@ -4113,6 +4086,29 @@ private static int doMain(String[] args) throws IOException, InterruptedExceptio
}
}
+ /** Class with static variables with information about CheckIndex's -level parameter. */
+ public static class Level {
+ private Level() {}
+
+ /** Minimum valid level. */
+ public static final int MIN_VALUE = 1;
+
+ /** Maximum valid level. */
+ public static final int MAX_VALUE = 3;
Review Comment:
Aha, here's my answer from first comment: `4` ("beyond slow") is not legal.
--
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
Re: [PR] CheckIndex - Adding a `-level` parameter to give ability to control index check detail programmatically [lucene]
Posted by "slow-J (via GitHub)" <gi...@apache.org>.
slow-J commented on PR #12797:
URL: https://github.com/apache/lucene/pull/12797#issuecomment-1821441651
Thanks for the review @mikemccand, will respond to the rest of the comments later!
--
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
Re: [PR] CheckIndex - Adding a `-level` parameter to give ability to control index check detail programmatically [lucene]
Posted by "slow-J (via GitHub)" <gi...@apache.org>.
slow-J commented on code in PR #12797:
URL: https://github.com/apache/lucene/pull/12797#discussion_r1400997736
##########
lucene/core/src/java/org/apache/lucene/index/CheckIndex.java:
##########
@@ -442,19 +442,19 @@ public void close() throws IOException {
IOUtils.close(writeLock);
}
- private boolean doSlowChecks;
+ private int level;
/**
- * If true, additional slow checks are performed. This will likely drastically increase time it
- * takes to run CheckIndex!
+ * Sets Level, the higher the value, the more additional checks are performed. This will likely
+ * drastically increase time it takes to run CheckIndex! See {@link Level}
*/
- public void setDoSlowChecks(boolean v) {
- doSlowChecks = v;
+ public void setLevel(int v) {
+ level = v;
Review Comment:
We do `throw IllegalArgumentException` if the input is below or above the limit :)
edit: see what you meant in a comment below, I now throw the exception from here too.
--
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