serhiy-bzhezytskyy commented on issue #10004:
URL: https://github.com/apache/lucene/issues/10004#issuecomment-5158494096
This thread reached a conclusion in 2019 that was never recorded, so the
issue reads as undecided when it is not. @jpountz:
> This feels too unsafe to me for CheckIndex. For instance, what if idField
is the corrupt field, you could end up with missing ids or the wrong ids? I'm
fine with adding more information to the CheckIndex status in order to make it
easier to do this kind of hacks on top of CheckIndex, but I'd like to keep
CheckIndex something that is rock solid.
and then:
> When I said "on top of CheckIndex", I was rather thinking of running
CheckIndex programmatically and then looking at the return value to understand
what segments might need salvaging. A separate stand-alone tool sounds good to
me too.
with @cpoerschke agreeing and leaving code pointers "for when this is being
returned to in the future".
Two things have happened since that are relevant.
**The "more information" now exists.** #16476 adds
`Status#brokenSegmentName`, so a caller can tell *which* segment's `.si` could
not be read rather than only that a commit point was unreadable. That is the
first field added to `Status` for this purpose since the ask, and it is what a
salvage tool built on top would need first.
**The separate tool has a second, independent motivation.** #7405 proposes
removing `-exorcise` outright, on the grounds that a destructive option inside
a diagnostic tool is a trap. That is the same conclusion reached from the
opposite direction: repair does not belong inside `CheckIndex`. Neither issue
references the other.
### What splitting would change
It is not only a matter of moving a method. `CheckIndex`'s public surface is
already part repair:
| stays with checking | belongs to repair |
|---|---|
| `clean`, `missingSegments`, `segmentsFileName`, `numSegments`,
`segmentsChecked`, `toolOutOfDate`, `segmentInfos`, `numBadSegments`,
`partial`, `maxSegmentName`, `validCounter` | `dir`, `newSegments`,
`totLoseDocCount` |
3 of 14 `Status` fields exist only so that something can be written.
`newSegments` is package-private with no accessor, which is why the guard in
#16474 has to live inside `exorciseIndex` — an external caller cannot check the
state itself.
And the constructor takes a write lock:
```java
public CheckIndex(Directory dir) throws IOException {
this(dir, dir.obtainLock(IndexWriter.WRITE_LOCK_NAME));
}
```
So a tool named "check" requires exclusive write access in order to look at
an index. A read-only checker would not need that, which makes this a change of
contract rather than a refactoring.
The cost looks smaller than the contract change suggests: `newSegments` and
`dir` have no readers outside `CheckIndex.java` itself, and `exorciseIndex` has
exactly one caller anywhere in the repository — `IndexUtils#tryRepairIndex` in
Luke.
### Names
If the split happens, the pair reads better as `IndexChecker` and
`IndexRepairer` than as `CheckIndex` plus something new: `IndexUpgrader`,
`IndexSplitter`, `IndexRearranger` and `IndexReorderer` all follow that form,
and `CheckIndex` is the only "verb + Index" among the five CLI tools. Keeping
the old name for a class whose contract no longer includes repair would be the
more confusing outcome. `IndexSalvager` is the other candidate, and it matches
the vocabulary of this issue — the operation drops documents rather than
recovering them, so a name that does not promise repair may be the more honest
one.
I have not measured what a downstream user might be reading from
`totLoseDocCount`, which is public; there are no readers in this repository,
but I did not check outside it.
Whether to do any of this is a call for the project. Recording the 2019
decision here would at least make the open set honest, and this issue can be
closed as "won't fix in CheckIndex" independently of whether a separate tool
ever gets written.
--
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]
