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

storage: fix checksum version check #37868

Merged
merged 3 commits into from
May 30, 2019
Merged

Conversation

tbg
Copy link
Member

@tbg tbg commented May 26, 2019

The check was sitting at evaluation time, where it is useless. It needs to
sit in the below-Raft code that actually computes the checksums.

This flew under the radar for quite some time, but was found in:

#37737 (comment)

thanks to the aggressive consistency checks we run in
version/mixed/nodes=3.

Release note: None

@tbg tbg requested a review from a team May 26, 2019 20:51
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@tbg tbg force-pushed the fix/checksum-versions branch from 35983e8 to ebcb12f Compare May 26, 2019 20:53
@tbg tbg requested a review from ajwerner May 26, 2019 20:57
@tbg tbg force-pushed the fix/checksum-versions branch from ebcb12f to 8d3deb2 Compare May 27, 2019 11:33
Copy link
Contributor

@ajwerner ajwerner left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 6 of 6 files at r1, 1 of 1 files at r2.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @tbg)


pkg/storage/replica_consistency_test.go, line 53 at r1 (raw file):

		rc, err := tc.repl.getChecksum(ctx, cc.ChecksumID)
		if !matchingVersion {
			require.True(t, testutils.IsError(err, "checksum is nil"))

If I were to ask for something on this change it'd be a comment about why the expected error here is what it is. I might have expected something about version mismatch case rather than just checksum is nil though it all makes sense after reading getChecksum.

tbg added 3 commits May 29, 2019 17:10
Checksum can be nil.

Release note: None
The check was sitting at evaluation time, where it is useless. It needs
to sit in the below-Raft code that actually computes the checksums.

This flew under the radar for quite some time, but was found in:

cockroachdb#37737 (comment)

thanks to the aggressive consistency checks we run in version/mixed/nodes=3.

Release note: None
@tbg tbg force-pushed the fix/checksum-versions branch from 8d3deb2 to e13298a Compare May 29, 2019 15:10
@tbg
Copy link
Member Author

tbg commented May 29, 2019

Done, TFTR!

bors r=ajwerner

@tbg
Copy link
Member Author

tbg commented May 30, 2019

bors r=ajwerner

craig bot pushed a commit that referenced this pull request May 30, 2019
37868: storage: fix checksum version check r=ajwerner a=tbg

The check was sitting at evaluation time, where it is useless. It needs to
sit in the below-Raft code that actually computes the checksums.

This flew under the radar for quite some time, but was found in:

#37737 (comment)

thanks to the aggressive consistency checks we run in
version/mixed/nodes=3.

Release note: None

Co-authored-by: Tobias Schottdorf <[email protected]>
@craig
Copy link
Contributor

craig bot commented May 30, 2019

Build succeeded

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants