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

fix(Test): Fix table checksum test. Test on uncompressed. #1952

Merged
merged 1 commit into from
May 19, 2023

Conversation

billprovince
Copy link
Contributor

@billprovince billprovince commented May 17, 2023

Problem

Fixes DGRAPHCORE-152

Previously, when testing to verify that a bad checksum would actually cause a checksum failure, we were overlooking the fact that our method of modifying the data could also modify the compressed data or even some of the metadata related to our block data.

The symptomatic results:

--- FAIL: TestTableChecksum (0.11s)
2640 table_test.go:698:
2641 Error Trace: table_test.go:698
2642 table_test.go:692
2643 Error: Received unexpected error:
2644 failed to initialize table error: failed to initialize biggest for table /tmp/2687635575.sst error: failed to decode compressed data in file: /tmp/2687635575.sst at offset: 51750, len: 149 error: failed to decompress err: unexpected EOF
2645 Test: TestTableChecksum

or some similar error

(There were earlier errors about failing to panic, but these were always generated before the secondary logged error was added).

Solution

Now, when we want to test that our checksum process is valid, we check with uncompressed data rather than with compressed data. In addition, we still write random bytes, but we no longer place it randomly. Instead, we place it in a location that will avoid the metadata locations of the data blocks.

@mangalaman93
Copy link
Contributor

This looks good to me but I will let @harshil-goel or @joshua-goldstein approve it. Thanks for the PR Bill.

Copy link
Contributor

@joshua-goldstein joshua-goldstein left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks Bill!

@billprovince billprovince merged commit 44b5978 into main May 19, 2023
7 checks passed
@billprovince billprovince deleted the FixTestTableChecksum branch May 19, 2023 04:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants