mikemccand commented on a change in pull request #128:
URL: https://github.com/apache/lucene/pull/128#discussion_r634640332
##########
File path: lucene/core/src/java/org/apache/lucene/index/CheckIndex.java
##########
@@ -216,6 +225,9 @@
/** Status of vectors */
public VectorValuesStatus vectorValuesStatus;
+
+ /** Status of soft deletes */
+ public SoftDeletsStatus softDeletesStatus;
Review comment:
Whoa, were we failing to `CheckIndex` soft deletes before this?
##########
File path: lucene/core/src/java/org/apache/lucene/index/CheckIndex.java
##########
@@ -926,17 +1100,19 @@ public Status checkIndex(List<String> onlySegments)
throws IOException {
* @lucene.experimental
*/
public static Status.LiveDocStatus testLiveDocs(
- CodecReader reader, PrintStream infoStream, boolean failFast) throws
IOException {
+ CodecReader reader, PrintStream infoStream, String segmentId) {
long startNS = System.nanoTime();
+ String segmentPartId = segmentId + "[LiveDocs]";
final Status.LiveDocStatus status = new Status.LiveDocStatus();
try {
- if (infoStream != null) infoStream.print(" test: check live
docs.....");
+ if (infoStream != null) infoStream.print(segmentPartId + " test:
check live docs.....");
final int numDocs = reader.numDocs();
if (reader.hasDeletions()) {
Bits liveDocs = reader.getLiveDocs();
if (liveDocs == null) {
- throw new RuntimeException("segment should have deletions, but
liveDocs is null");
+ throw new RuntimeException(
Review comment:
Should we maybe make a new `RuntimeException` subclass, that takes this
`segmentPartId` as its own `String` parameter, and the exception message, and
maybe a `Throwable cause`?
##########
File path: lucene/core/src/java/org/apache/lucene/index/CheckIndex.java
##########
@@ -468,6 +495,10 @@ private static void msg(PrintStream out, String msg) {
if (out != null) out.println(msg);
}
+ private static void msg(PrintStream out, String id, String msg) {
+ if (out != null) out.println(id + " " + msg);
Review comment:
Could you break this into separate lines and add `{ ... }`?
##########
File path: lucene/core/src/java/org/apache/lucene/index/CheckIndex.java
##########
@@ -926,17 +1100,19 @@ public Status checkIndex(List<String> onlySegments)
throws IOException {
* @lucene.experimental
*/
public static Status.LiveDocStatus testLiveDocs(
- CodecReader reader, PrintStream infoStream, boolean failFast) throws
IOException {
+ CodecReader reader, PrintStream infoStream, String segmentId) {
long startNS = System.nanoTime();
+ String segmentPartId = segmentId + "[LiveDocs]";
final Status.LiveDocStatus status = new Status.LiveDocStatus();
try {
- if (infoStream != null) infoStream.print(" test: check live
docs.....");
+ if (infoStream != null) infoStream.print(segmentPartId + " test:
check live docs.....");
Review comment:
Sorry about not answering the `// nocommit` question before.
Ideally, all `infoStream.print` for a given "part" of the index checking
would first append to a per-part log, and then (under lock) print to
console/main infoStream as a single "block" of output? (So that we don't see
confusing interleaved across segments/parts checks)?
But I think it is OK to make this (cosmetic) improvement as a followon PR
... this change is already awesome enough.
##########
File path: lucene/core/src/java/org/apache/lucene/index/CheckIndex.java
##########
@@ -372,6 +384,14 @@ private FieldNormStatus() {}
/** Exception thrown during term index test (null on success) */
public Throwable error = null;
}
+
+ /** Status from testing soft deletes */
+ public static final class SoftDeletsStatus {
+ SoftDeletsStatus() {}
+
+ /** Exception thrown during soft deletes test (null on success) */
+ public Throwable error = null;
Review comment:
Hmm we don't need the `= null` -- it is already java's default.
##########
File path: lucene/core/src/java/org/apache/lucene/index/CheckIndex.java
##########
@@ -2106,16 +2286,6 @@ static void checkImpacts(Impacts impacts, int
lastTarget) {
}
}
- /**
- * Test the term index.
- *
- * @lucene.experimental
- */
- public static Status.TermIndexStatus testPostings(CodecReader reader,
PrintStream infoStream)
Review comment:
Hmm, did this just move elsewhere? This is a helpful API to test just
postings ...
##########
File path: lucene/core/src/java/org/apache/lucene/index/CheckIndex.java
##########
@@ -2737,13 +2910,14 @@ public Relation compare(byte[] minPackedValue, byte[]
maxPackedValue) {
* @lucene.experimental
*/
public static Status.StoredFieldStatus testStoredFields(
- CodecReader reader, PrintStream infoStream, boolean failFast) throws
IOException {
+ CodecReader reader, PrintStream infoStream, String segmentId) {
long startNS = System.nanoTime();
+ String segmentPartId = segmentId + "[StoredFields]";
Review comment:
OK maybe the custom `RuntimeException` subclass could take
`CheckIndexFailure(String segmentId, String indexPart, String message,
Throwable rootCause) {...}`?
##########
File path: lucene/core/src/java/org/apache/lucene/index/CheckIndex.java
##########
@@ -2795,12 +2972,14 @@ public Relation compare(byte[] minPackedValue, byte[]
maxPackedValue) {
* @lucene.experimental
*/
public static Status.DocValuesStatus testDocValues(
- CodecReader reader, PrintStream infoStream, boolean failFast) throws
IOException {
Review comment:
This is net/net an API break, but given that `CheckIndex` is
internal/experimental, I think it is OK (to backport to Lucene 8.x 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.
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
