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

roachtest: move CheckReplicaDivergenceOnDB to util #87383

Merged
merged 1 commit into from
Sep 8, 2022

Conversation

tbg
Copy link
Member

@tbg tbg commented Sep 5, 2022

This had no business sitting on Cluster.

Release note: None
Release justification: testing code

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@tbg tbg marked this pull request as ready for review September 5, 2022 12:16
@tbg tbg requested a review from a team as a code owner September 5, 2022 12:16
@tbg tbg requested review from srosenberg and removed request for a team September 5, 2022 12:16
Copy link
Contributor

@renatolabs renatolabs left a comment

Choose a reason for hiding this comment

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

:lgtm:

Thanks for refactoring! I had seen this code in the past and thought it was weird that it was a method on Cluster.

Since we're looking at this code, I wanted to double check that the comments in that function are still accurate (that we need to set a timeout explicitly, and that the check can fail easily). @tbg would you know if that's still the case?

And a more general question: currently this function is only used by one upgrade test that doesn't use the versionUpgradeTest struct. Would this consistency check be useful in the other upgrade tests as well?

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @srosenberg)

Copy link
Member Author

@tbg tbg left a comment

Choose a reason for hiding this comment

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

Since we're looking at this code, I wanted to double check that the comments in that function are still accurate (that we need to set a timeout explicitly, and that the check can fail easily). @tbg would you know if that's still the case?

They are, but I have a string of commits that make them more useful: #87378
TFTR!
bors r=renatolabs

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @srosenberg)

@craig
Copy link
Contributor

craig bot commented Sep 7, 2022

👎 Rejected by PR status

@tbg
Copy link
Member Author

tbg commented Sep 7, 2022

bors r-

This had no business sitting on `Cluster`.

Release note: None
@tbg tbg force-pushed the check-repl-divergence-roachtest branch from 5b9f525 to ecaf9c8 Compare September 7, 2022 15:12
@tbg
Copy link
Member Author

tbg commented Sep 7, 2022

eslint failed on PR CI, but that's not this PR's fault, discussion here.

bors r=renatolabs

@craig
Copy link
Contributor

craig bot commented Sep 7, 2022

This PR was included in a batch that successfully built, but then failed to merge into master. It will not be retried.

Additional information:

{"message":"1 review requesting changes by reviewers with write access.","documentation_url":"https://docs.github.com/articles/about-protected-branches"}

@tbg
Copy link
Member Author

tbg commented Sep 8, 2022

bors r=renatolabs

@craig
Copy link
Contributor

craig bot commented Sep 8, 2022

Build failed:

@tbg
Copy link
Member Author

tbg commented Sep 8, 2022

bors r=renatolabs

@craig
Copy link
Contributor

craig bot commented Sep 8, 2022

Build succeeded:

@craig craig bot merged commit d33e93f into cockroachdb:master Sep 8, 2022
@tbg tbg deleted the check-repl-divergence-roachtest branch September 8, 2022 14:01
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