Don't test corruption detection within CFS checksum #33911
Conversation
vladimirdolzhenko
added
:Distributed/Store
|
Pinging @elastic/es-distributed |
|
With and manual position adjustment: CheckIndex does not detect corruption on flipping at position 2517..2520 on file: _0.cfs, length: 2521 CheckIndex does detect corruption on flipping at position 2516 file: _0.cfs, length: 2521 |
vladimirdolzhenko
force-pushed
the
fix/33881
branch
from
e408902
to
5f5874d
Compare
vladimirdolzhenko
requested review from
ywelsch and
DaveCTurner
and removed request for
ywelsch
There was a problem hiding this comment.
Good catch!
Could we have a test that fails if this issue is fixed in Lucene? Otherwise it could be fixed without anyone noticing and then we'd have a gap in our coverage.
I've run a few thousand iterations of these tests with this fix in place without finding an issue, but it also seems tricky to reproduce without the fix, so I'm keeping it running for a while.
|
|
||
| if (fileToCorrupt.getFileName().toString().endsWith(".cfs") && maxPosition > Integer.BYTES) { | ||
| // TODO: it is known that Lucene does not check the tail of CFS file (CompoundFileS, like an archive), see note at | ||
| // https://github.com/apache/lucene-solr/blob/branch_7_5/lucene/core/src/java/org/apache/lucene/codecs/lucene50/ |
There was a problem hiding this comment.
Could you make this link into the following so that it doesn't rot as the code changes:
In fact it might be better to just put this link in the body of the first post of this PR, and make the code comment be a link to this PR, as it'll be much shorter.
There was a problem hiding this comment.
updated link in PR description (it was pointed to a single line)
the problem with the link that it violates checkstyle 140 chars max per line, will add link to this PR as it way shorter 💯
| // https://github.com/apache/lucene-solr/blob/branch_7_5/lucene/core/src/java/org/apache/lucene/codecs/lucene50/ | ||
| // Lucene50CompoundReader.java#L82 | ||
| // so far, don't corrupt checksum (last 4 bytes) of cfs file | ||
| maxPosition -= Integer.BYTES; |
There was a problem hiding this comment.
This is a little confusing: the checksum is actually the last 8 bytes of the file, but the first 4 of those should be 0 (and we verify that). Could (a) the comment be more detailed and (b) the Integer.BYTES just say 4 to avoid this confusion?
DaveCTurner
changed the title
|
The magic words are "test this please" :) (but not magic enough to fix PR builds it seems) |
|
@elasticmachine test this please |
|
thanks @DaveCTurner for the comments |
It is known that Lucene does not check the tail of CFS file (CompoundFileS, like an archive) - see note at https://github.com/apache/lucene-solr/blob/60e8ee717d9cd8d2425c100c21168d6a43b78516/lucene/core/src/java/org/apache/lucene/codecs/lucene50/Lucene50CompoundReader.java#L82-L85
Therefore if we'd like Lucene checkIndex to detect corruption we should not corrupt checksum (last 4 bytes) of cfs file.
Closes #33881