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

Verify Checksum once it has been fully written to fail as soon as possible #13896

Merged
merged 1 commit into from Oct 2, 2015

Conversation

Projects
None yet
3 participants
@s1monw
Copy link
Contributor

commented Oct 1, 2015

Today we are relying on calling Store.verify on the closed stream to validate the
checksum. This is still necessary to catch file truncation but for an actually corrupted
file or checksum we can fail early and check the checksum against the actual metadata
once it's been fully written to the VerifyingIndexOutput.

This failure is fixed with this PR:

http://build-us-00.elastic.co/job/es_core_master_strong/5179/testReport/junit/org.elasticsearch.indices.recovery/RecoverySourceHandlerTests/testHandleCorruptedIndexOnSendSendFiles/
@jasontedor

This comment has been minimized.

Copy link
Member

commented Oct 2, 2015

LGTM.

Verify Checksum once it has been fully written to fail as soon as pos…
…sible

Today we are relying on calling Store.verify on the closed stream to validate the
checksum. This is still necessary to catch file truncation but for an actually corrupted
file or checksum we can fail early and check the checksum against the actual metadata
once it's been fully written to the VerifyingIndexOutput.

@s1monw s1monw force-pushed the s1monw:verify_once_recorded branch to 04e8926 Oct 2, 2015

@s1monw s1monw merged commit 04e8926 into elastic:master Oct 2, 2015

1 check passed

CLA Commit author is a member of Elasticsearch
Details
@s1monw

This comment has been minimized.

Copy link
Contributor Author

commented Oct 2, 2015

I will wait a bit until I backport this just to make sure it doesn't break anything

s1monw added a commit to s1monw/elasticsearch that referenced this pull request Oct 4, 2015

Record all bytes of the checksum in VerifyingIndexOutput
The fix in elastic#13848 has an off by one issue where the first byte of the checksum
was never written. Unfortunately most tests shadowed the problem and the first
byte of the checksum seems to be very likely a 0 which causes only very rare
failures.

Relates to elastic#13896
Relates to elastic#13848

s1monw added a commit that referenced this pull request Oct 5, 2015

Record all bytes of the checksum in VerifyingIndexOutput
The fix in #13848 has an off by one issue where the first byte of the checksum
was never written. Unfortunately most tests shadowed the problem and the first
byte of the checksum seems to be very likely a 0 which causes only very rare
failures.

Relates to #13896
Relates to #13848

s1monw added a commit that referenced this pull request Oct 5, 2015

Record all bytes of the checksum in VerifyingIndexOutput
The fix in #13848 has an off by one issue where the first byte of the checksum
was never written. Unfortunately most tests shadowed the problem and the first
byte of the checksum seems to be very likely a 0 which causes only very rare
failures.

Relates to #13896
Relates to #13848
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.