serhiy-bzhezytskyy commented on issue #7823:
URL: https://github.com/apache/lucene/issues/7823#issuecomment-5158620831
Still reproducible on `main`, and after ten years three things about it have
changed or turned out differently than the description assumes.
**The field this asks for no longer exists.** `Status.cantOpenSegments` was
removed in 2018 by LUCENE-8437 (`746c9f7c225`, *"CheckIndex shouldn't duplicate
SegmentInfos' serialization logic"*), three years after this was filed. The fix
suggested here — return a status with that flag set — is no longer available as
written.
**Returning a `Status` instead of throwing would diverge from the rest of
the API.** `IndexNotFoundException` on a directory with no `segments_N` is not
specific to `CheckIndex`: `SegmentInfos` throws the same thing on the same
condition (`gen == -1`, `SegmentInfos.java:821`), which is what
`DirectoryReader.open` surfaces. So `checkIndex()` is currently consistent with
its neighbours, and changing it would make `DirectoryReader.open(dir)` throw
while `new CheckIndex(dir).checkIndex()` does not. Also `missingSegments` means
"the `segments_N` could not be read", which is a different condition from
"there is nothing to read" — folding both into one flag would lose the
distinction this issue is about.
**What is rough is the command line, not the API.** `doCheck` reads the
result and returns an exit code rather than catching, so
`IndexNotFoundException` passes through `doCheck`, `doMain` and `main`, and the
user gets a stack trace. Measured on `main`, for three inputs:
| input | result |
|---|---|
| empty directory | `IndexNotFoundException`, stack trace |
| non-existent path | same — and the directory now exists, containing
`write.lock` |
| directory with unrelated files | same |
The command line already prints a clean message and returns 1 for every
other failure, including *"could not open directory"* a few lines earlier, so
the stack trace here looks like an oversight rather than a decision. The
created directory and `write.lock` come from `FSDirectory`, which does
`Files.createDirectories(path)` in its constructor (`FSDirectory.java:122`) —
that is documented behaviour of `FSDirectory`, not something `CheckIndex` does,
and the CLI opens the `Directory` itself before constructing `CheckIndex`.
**And this is the same gap as #7820, from the other side.** That issue is
about a commit point that exists but cannot be read; this one about none
existing at all. Both leave the caller unable to tell *why* from a `Status`.
The two were filed one day apart in August 2015, by different people, and
neither has referenced the other. They are adjacent in the code too — the
`throw` reported here is at `CheckIndex.java:628`, and the comment on the next
line is about #7820:
```java
if (lastSegmentsFile == null) {
throw new IndexNotFoundException(
"no segments* file found in " + dir + ": files: " +
Arrays.toString(files));
}
// https://github.com/apache/lucene/issues/7820: also attempt to open
any older commit
// points (segments_N), which will catch certain corruption like missing
_N.si files
```
From the #7820 side, #16476 adds `Status#brokenSegmentName` so a caller can
tell which segment's `.si` was unreadable — the same shape of answer, but it
does not cover this case, since there is no commit point from which to name a
segment.
For the reporting use case in the description, catching
`IndexNotFoundException` is what any other caller of a Lucene open-an-index API
has to do. So the API change asked for here looks unnecessary; a clean CLI
message would be a small separate improvement, if wanted.
--
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]
