Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Core: Don't verify adler32 for lucene 3.x terms dict/terms index. #9142

Closed
wants to merge 6 commits into from

Conversation

@rmuir
Copy link
Contributor

commented Jan 5, 2015

These files are not write-once, but the 0.20.x checksum code
didn't handle this properly, so we can't use the adler32.

Don't verify adler32 for lucene 3.x terms dict/terms index.
These files are not write-once, but the 0.20.x checksum code
didn't handle this properly, so we can't use the adler32.
@rjernst

This comment has been minimized.

Copy link
Member

commented Jan 5, 2015

LGTM.

@rmuir rmuir added the v1.5.0 label Jan 5, 2015

rmuir added 2 commits Jan 5, 2015
@rmuir

This comment has been minimized.

Copy link
Contributor Author

commented Jan 5, 2015

I added a commit enabling testing of 0.20.6 index. It fails with the checksum error without this fix.

@rmuir rmuir added the review label Jan 5, 2015

@rjernst

This comment has been minimized.

Copy link
Member

commented Jan 5, 2015

Still looks good!

@rmuir

This comment has been minimized.

Copy link
Contributor Author

commented Jan 6, 2015

I thought about this .CFS case all day. For now, I would like the additional added logic (see latest commit) to avoid false checksum failures. If we want to throw an error on old versions i would like to separate that from doing the right thing here. Also note again we don't need this cruft on master, just 1.x

@rmuir

This comment has been minimized.

Copy link
Contributor Author

commented Jan 6, 2015

Obviously this one is not really easy to test, our backwards tests config currently does not work with such ancient versions. But I know these old segments are still floating around, because I see people still hitting https://issues.apache.org/jira/browse/LUCENE-5975 on 1.3.x versions and so on. Enjoy reviewing :)

@s1monw

This comment has been minimized.

Copy link
Contributor

commented Jan 6, 2015

looks good though. can we maybe have a simple unittest in StoreTests that checks if your conditionals are working ie with some bogus StoreFileMetadata?

@rmuir

This comment has been minimized.

Copy link
Contributor Author

commented Jan 6, 2015

Good idea. We might as well put that fake segmentinfowriter to use.

rmuir added 2 commits Jan 6, 2015
@rmuir

This comment has been minimized.

Copy link
Contributor Author

commented Jan 6, 2015

I added tests for these cases with fake metadata.

@s1monw

This comment has been minimized.

Copy link
Contributor

commented Jan 6, 2015

very cool thanks rob! LGTM

rmuir added a commit that referenced this pull request Jan 6, 2015
Don't verify adler32 for buggy legacy checksums.
Don't verify adler32 for lucene 3.x terms dictionary (.tis),
terms index (.tii), 3.0-3.3 compound file (.cfs), 3.x segments_N,
or 3.x segments.gen.

These files are not write-once, but the 0.20.x checksum code
didn't handle .tis/.tii properly, so we can't use the adler32.
Even older versions will have the same problems with other cases.

Closes #9142
rmuir added a commit that referenced this pull request Jan 6, 2015
Don't verify adler32 for buggy legacy checksums.
Don't verify adler32 for lucene 3.x terms dictionary (.tis),
terms index (.tii), 3.0-3.3 compound file (.cfs), 3.x segments_N,
or 3.x segments.gen.

These files are not write-once, but the 0.20.x checksum code
didn't handle .tis/.tii properly, so we can't use the adler32.
Even older versions will have the same problems with other cases.

Closes #9142

Conflicts:
	src/main/java/org/elasticsearch/index/store/Store.java
	src/test/java/org/elasticsearch/bwcompat/OldIndexBackwardsCompatibilityTests.java
rmuir added a commit that referenced this pull request Jan 6, 2015
Don't verify adler32 for buggy legacy checksums.
Don't verify adler32 for lucene 3.x terms dictionary (.tis),
terms index (.tii), 3.0-3.3 compound file (.cfs), 3.x segments_N,
or 3.x segments.gen.

These files are not write-once, but the 0.20.x checksum code
didn't handle .tis/.tii properly, so we can't use the adler32.
Even older versions will have the same problems with other cases.

Closes #9142

Conflicts:
	src/main/java/org/elasticsearch/index/store/Store.java
	src/test/java/org/elasticsearch/bwcompat/OldIndexBackwardsCompatibilityTests.java

@rmuir rmuir closed this Jan 6, 2015

@clintongormley clintongormley changed the title Don't verify adler32 for lucene 3.x terms dict/terms index. Core: Don't verify adler32 for lucene 3.x terms dict/terms index. Feb 10, 2015

mute pushed a commit to mute/elasticsearch that referenced this pull request Jul 29, 2015
Don't verify adler32 for buggy legacy checksums.
Don't verify adler32 for lucene 3.x terms dictionary (.tis),
terms index (.tii), 3.0-3.3 compound file (.cfs), 3.x segments_N,
or 3.x segments.gen.

These files are not write-once, but the 0.20.x checksum code
didn't handle .tis/.tii properly, so we can't use the adler32.
Even older versions will have the same problems with other cases.

Closes elastic#9142

Conflicts:
	src/main/java/org/elasticsearch/index/store/Store.java
	src/test/java/org/elasticsearch/bwcompat/OldIndexBackwardsCompatibilityTests.java
mute pushed a commit to mute/elasticsearch that referenced this pull request Jul 29, 2015
Don't verify adler32 for buggy legacy checksums.
Don't verify adler32 for lucene 3.x terms dictionary (.tis),
terms index (.tii), 3.0-3.3 compound file (.cfs), 3.x segments_N,
or 3.x segments.gen.

These files are not write-once, but the 0.20.x checksum code
didn't handle .tis/.tii properly, so we can't use the adler32.
Even older versions will have the same problems with other cases.

Closes elastic#9142

Conflicts:
	src/main/java/org/elasticsearch/index/store/Store.java
	src/test/java/org/elasticsearch/bwcompat/OldIndexBackwardsCompatibilityTests.java
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.