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.

Sign up for GitHub

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

Jump to bottom

streamingest: unskip TestTenantStreamingUnavailableStreamAddress #107094

Merged
merged 1 commit into from
Jul 28, 2023
Merged

streamingest: unskip TestTenantStreamingUnavailableStreamAddress #107094

merged 1 commit into from
Jul 28, 2023

Conversation

lidorcarmel
Copy link
Contributor

@lidorcarmel lidorcarmel commented Jul 18, 2023 •
edited
Loading

Changing a few things to get this test to pass under stress:

Epic: none
Fixes: #107023
Fixes: #106865

Release note: None

@lidorcarmel lidorcarmel requested a review from a team as a code owner July 18, 2023 18:16
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@lidorcarmel lidorcarmel marked this pull request as draft July 18, 2023 18:16
@lidorcarmel lidorcarmel force-pushed the lidor_c2c_unskip_mt_issue branch 2 times, most recently from bcf1f9c to 954cada Compare July 24, 2023 21:50
@lidorcarmel lidorcarmel marked this pull request as ready for review July 24, 2023 21:52
stevendanna
stevendanna approved these changes Jul 25, 2023
View reviewed changes
Copy link
Collaborator

@stevendanna stevendanna left a comment

Choose a reason for hiding this comment

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

LGTM.

If you wanted, there isn't really a need to call StartShaedProcessTenant again. We could open a connection to Server(2) which should still have a tenant running.

But, if you've already stressed this, let's stick with this.

@lidorcarmel
Copy link
Contributor Author

thanks, good idea but it flakes with that :(
anyway it made me realize I'm missing a wait after starting the alternative tenant, added now.
stressed again, still looks good.

bors r+

I'm happy to do followups, I think there are a few small things I want to cleanup here.

@craig
Copy link
Contributor

craig bot commented Jul 25, 2023

This PR was included in a batch that was canceled, it will be automatically retried

@craig
Copy link
Contributor

craig bot commented Jul 25, 2023

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Jul 25, 2023

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Jul 25, 2023

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Jul 25, 2023

Build failed (retrying...):

@nvanbenschoten
Copy link
Member

It looks like TestTenantStreamingUnavailableStreamAddress failed in both of the previous two bors runs, so this test isn't quite ready to be unskipped. I'll remove from the queue for now.

bors r-

@craig
Copy link
Contributor

craig bot commented Jul 25, 2023

Canceled.

@lidorcarmel
Copy link
Contributor Author

bummer. not ready indeed. thanks Nathan and sorry about that! I guess local stress was not enough here :(
btw the first failure out of the 3 above was a different one: #107434
I'll fix.

@lidorcarmel
Copy link
Contributor Author

@stevendanna it's ready for you.. not touching the source cluster at all now, after killing a server.
stressed 1000+ runs 🤞

stevendanna
stevendanna approved these changes Jul 27, 2023
View reviewed changes
Copy link
Collaborator

@stevendanna stevendanna left a comment

Choose a reason for hiding this comment

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

Let's try this again :D. Thanks for digging into it.

@lidorcarmel
Copy link
Contributor Author

thanks!

bors r+

@craig
Copy link
Contributor

craig bot commented Jul 28, 2023

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Jul 28, 2023

Build failed:

@lidorcarmel
Copy link
Contributor Author

a rebase was needed, it should pass now.

bors r+

@lidorcarmel
streamingest: unskip TestTenantStreamingUnavailableStreamAddress
ce27952 
Changing a few things to get this test to pass under stress:
- use 50 ranges instead of 10, because there are already 50-ish system ranges,
  so if we write only 10 more ranges those might not get distributed on all
  servers.
- avoid reading from the source cluster after stopping a node, it's flaky,
  see cockroachdb#107499 for more info.

Epic: none
Fixes: cockroachdb#107023
Fixes: cockroachdb#106865

Release note: None
@craig
Copy link
Contributor

craig bot commented Jul 28, 2023

Canceled.

@lidorcarmel
Copy link
Contributor Author

bors r+

@craig craig bot merged commit 04c91a5 into cockroachdb:master Jul 28, 2023
2 checks passed
@craig
Copy link
Contributor

craig bot commented Jul 28, 2023

Build succeeded:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stevendanna stevendanna stevendanna approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@lidorcarmel @cockroach-teamcity @nvanbenschoten @stevendanna