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

Add sanity check to InlineVerifier and TargetVerifier #261

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Lincoln23
Copy link

@Lincoln23 Lincoln23 commented Feb 22, 2021

#263

For InlineVerifier:
check if rowsChecked > 0 when we run VerifyDuringCutover.
if no rows were checked, return VerificationResult.DataCorrect = False

For TargetVerifier:
check after we run StopTargetVerifier to see if we verified any events
if no events were checked, we log and return an error, copydb and sharding will then panic

@Lincoln23 Lincoln23 marked this pull request as draft February 22, 2021 21:01
@Lincoln23 Lincoln23 force-pushed the add-row-verified-check branch 7 times, most recently from 3750b1c to d131335 Compare February 24, 2021 17:23
@Lincoln23 Lincoln23 changed the title Add RowsVerified to InlineVerifier and EventsVerified to TargetVerifier Add sanity check to InlineVerifier and TargetVerifier Feb 24, 2021
@@ -68,7 +68,7 @@ def test_different_compressed_data_is_detected_inline_with_batch_writer

assert verification_ran
assert_equal ["#{DEFAULT_DB}.#{DEFAULT_TABLE}"], incorrect_tables
assert_equal "cutover verification failed for: gftest.test_table_1 [paginationKeys: 1 ] ", ghostferry.error_lines.last["msg"]
assert_equal "cutover verification failed for: gftest.test_table_1 [paginationKeys: 1 ] ", ghostferry.error_lines.first["msg"]
Copy link
Author

@Lincoln23 Lincoln23 Feb 24, 2021

Choose a reason for hiding this comment

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

These tests will log target verifier did not check any events since there are no events on the target. I added a separate test to check target verifier events below so I will ignore that error here

Same changes for tests below

Comment on lines +439 to +444
if v.rowsChecked == 0 {
return VerificationResult{
DataCorrect: false,
Message: "cutover verification failed, no rows were compared",
}, nil
}
Copy link
Author

@Lincoln23 Lincoln23 Feb 24, 2021

Choose a reason for hiding this comment

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

I wasn't sure how to test this in the ruby integration tests since in integrationferry here we only pass the incorrect tables. perhaps we pass the message as well?

@Lincoln23 Lincoln23 closed this Feb 24, 2021
@Lincoln23 Lincoln23 reopened this Feb 24, 2021
@Lincoln23 Lincoln23 marked this pull request as ready for review February 24, 2021 18:32
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.

None yet

1 participant