Skip to content

streamingest: unskip TestTenantStreamingUnavailableStreamAddress#107094

Merged
craig[bot] merged 1 commit intocockroachdb:masterfrom
lidorcarmel:lidor_c2c_unskip_mt_issue
Jul 28, 2023
Merged

streamingest: unskip TestTenantStreamingUnavailableStreamAddress#107094
craig[bot] merged 1 commit intocockroachdb:masterfrom
lidorcarmel:lidor_c2c_unskip_mt_issue

Conversation

@lidorcarmel
Copy link
Copy Markdown
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 stevendanna July 18, 2023 18:16
@lidorcarmel lidorcarmel requested a review from a team as a code owner July 18, 2023 18:16
@cockroach-teamcity
Copy link
Copy Markdown
Member

cockroach-teamcity commented Jul 18, 2023

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
Copy Markdown
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 lidorcarmel force-pushed the lidor_c2c_unskip_mt_issue branch from 954cada to 6761983 Compare July 25, 2023 16:30
@lidorcarmel
Copy link
Copy Markdown
Contributor Author

lidorcarmel commented Jul 25, 2023

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
Copy Markdown
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
Copy Markdown
Contributor

craig bot commented Jul 25, 2023

Build failed (retrying...):

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Jul 25, 2023

Build failed (retrying...):

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Jul 25, 2023

Build failed (retrying...):

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Jul 25, 2023

Build failed (retrying...):

@nvb
Copy link
Copy Markdown
Contributor

nvb commented Jul 25, 2023

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
Copy Markdown
Contributor

craig bot commented Jul 25, 2023

Canceled.

@lidorcarmel
Copy link
Copy Markdown
Contributor Author

lidorcarmel commented Jul 25, 2023

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 lidorcarmel force-pushed the lidor_c2c_unskip_mt_issue branch from 6761983 to 094ec29 Compare July 27, 2023 15:11
@lidorcarmel
Copy link
Copy Markdown
Contributor Author

lidorcarmel commented Jul 27, 2023

@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
Copy Markdown
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
Copy Markdown
Contributor Author

lidorcarmel commented Jul 27, 2023

thanks!

bors r+

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Jul 28, 2023

Build failed (retrying...):

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Jul 28, 2023

Build failed:

@lidorcarmel lidorcarmel force-pushed the lidor_c2c_unskip_mt_issue branch from 094ec29 to af4f1c8 Compare July 28, 2023 16:42
@lidorcarmel
Copy link
Copy Markdown
Contributor Author

lidorcarmel commented Jul 28, 2023

a rebase was needed, it should pass now.

bors r+

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
@lidorcarmel lidorcarmel force-pushed the lidor_c2c_unskip_mt_issue branch from af4f1c8 to ce27952 Compare July 28, 2023 17:30
@craig
Copy link
Copy Markdown
Contributor

craig bot commented Jul 28, 2023

Canceled.

@lidorcarmel
Copy link
Copy Markdown
Contributor Author

lidorcarmel commented Jul 28, 2023

bors r+

@craig craig bot merged commit 04c91a5 into cockroachdb:master Jul 28, 2023
@craig
Copy link
Copy Markdown
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

Development

Successfully merging this pull request may close these issues.

ccl/streamingccl/streamingest: TestTenantStreamingUnavailableStreamAddress failed c2c: TestTenantStreamingUnavailableStreamAddress flaked on master

4 participants

@lidorcarmel @cockroach-teamcity @nvb @stevendanna