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

multiregionccl: improve and deflake TestMultiRegionDataDriven #86339

Merged
merged 1 commit into from
Aug 18, 2022

Conversation

arulajmani
Copy link
Collaborator

@arulajmani arulajmani commented Aug 17, 2022

fixes #77908
fixes #80837

This patch switches to printing the entire trace instead of an opaque
failure if the trace analysis does not conform to the expected output.

We also deflake this test. The framework allows for tests to express
an expected leaseholder and wait for these changes to apply. Given the
test cluster interface, the only claim that can be made here is that
the outgoing leaseholder has applied the lease transfer. Other replicas
may still be operating under a stale view of the lease. The
"served via follower read" test output line is contingent on the serving
replicas view of the lease. I think this was leading to some (but maybe
not all) flakes we've seen sporadically. To get around this, we now wait
for all replicas to have the same view of the leaseholder when waiting
for zone config changes to apply.

Release justification: non production code change
Release note: None

@arulajmani arulajmani requested a review from a team as a code owner August 17, 2022 21:33
@cockroach-teamcity
Copy link
Member

This change is Reviewable

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:

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

@arulajmani
Copy link
Collaborator Author

TFTR!

bors r=ajwerner

@craig
Copy link
Contributor

craig bot commented Aug 17, 2022

🕐 Waiting for PR status (Github check) to be set, probably by CI. Bors will automatically try to run when all required PR statuses are set.

@craig
Copy link
Contributor

craig bot commented Aug 17, 2022

GitHub status checks took too long to complete, so bors is giving up. You can adjust bors configuration to have it wait longer if you like.

This patch switches to printing the entire trace instead of an opaque
failure if the trace analysis does not conform to the expected output.

We also deflake this test. The framework allows for tests to express
an expected leaseholder and wait for these changes to apply. Given the
test cluster interface, the only claim that can be made here is that
the outgoing leaseholder has applied the lease transfer. Other replicas
may still be operating under a stale view of the lease. The
"served via follower read" test output line is contingent on the serving
replicas view of the lease. I think this was leading to some (but maybe
not all) flakes we've seen sporadically. To get around this, we now wait
for all replicas to have the same view of the leaseholder when waiting
for zone config changes to apply.

Release justification: non production code change
Release note: None
@arulajmani arulajmani force-pushed the multi-region-data-driven-test branch from d4706fd to e65b3ca Compare August 18, 2022 02:11
@arulajmani
Copy link
Collaborator Author

bors r=ajwerner

@craig
Copy link
Contributor

craig bot commented Aug 18, 2022

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.

ccl/multiregionccl: TestMultiRegionDataDriven failed ccl/multiregionccl: TestMultiRegionDataDriven failed
4 participants