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

changfeedccl: deflake TestAlterChangefeedTelemetry #91884

Merged
merged 2 commits into from
Nov 15, 2022

Conversation

stevendanna
Copy link
Collaborator

@stevendanna stevendanna commented Nov 15, 2022

The job system clears the lease asyncronously after cancelation. This
lease clearing transaction can cause a restart in the alter changefeed
transaction, which will lead to different feature counter
counts. Thus, we want to wait for the lease clear. However, the lease
clear isn't guaranteed to happen, so we only wait a few seconds for
it.

Epic: None

Release note: None

@stevendanna stevendanna requested a review from a team as a code owner November 15, 2022 00:14
@cockroach-teamcity
Copy link
Member

This change is Reviewable

The job system clears the lease asyncronously after cancelation. This
lease clearing transaction can cause a restart in the alter changefeed
transaction, which will lead to different feature counter
counts. Thus, we want to wait for the lease clear. However, the lease
clear isn't guaranteed to happen, so we only wait a few seconds for
it.

Release note: None
@stevendanna stevendanna requested a review from a team as a code owner November 15, 2022 02:21
@stevendanna stevendanna requested review from HonoreDB and removed request for a team November 15, 2022 02:21
@stevendanna
Copy link
Collaborator Author

@miretskiy I pushed another commit to de-flake the test where I found this. Mind taking a look.

@stevendanna stevendanna changed the title testutils: add job id to error message changfeedccl: deflake TestAlterChangefeedTelemetry Nov 15, 2022
@stevendanna
Copy link
Collaborator Author

bors r+

I'm going to merge this to try to relieve the impact of the flake, we can follow up with a better fix if necessary.

// clear. However, the lease clear isn't guaranteed to
// happen, so we only wait a few seconds for it.
waitForNoLease := func() {
ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should the period be increased if under race?
helpers_test.go has withTimeout function that can help with this, plus assertPayloadsTimeout() is a helper that returns timeout adjusted for race (might need renaming)

@stevendanna
Copy link
Collaborator Author

bors r+

@craig
Copy link
Contributor

craig bot commented Nov 15, 2022

Build succeeded:

@craig craig bot merged commit 5a24e11 into cockroachdb:master Nov 15, 2022
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