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

streamingccl: reduce server count in multinode tests #87027

Merged

Conversation

samiskin
Copy link
Contributor

While these tests would pass under stress locally they would fail CI
stress, which may be because we were starting more server processes than
ever before with 4 source nodes, 4 source tenant pods, and 4 destination
nodes.

This PR reduces the node count to 3 (any lower and scatter doesn't
correctly distribute ranges) and only starts a single tenant pod for the
source cluster.

Release justification: test-only change
Release note: None

While these tests would pass under stress locally they would fail CI
stress, which may be because we were starting more server processes than
ever before with 4 source nodes, 4 source tenant pods, and 4 destination
nodes.

This PR reduces the node count to 3 (any lower and scatter doesn't
correctly distribute ranges) and only starts a single tenant pod for the
source cluster.

Release justification: test-only change
Release note: None
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@samiskin samiskin marked this pull request as ready for review August 29, 2022 16:17
@samiskin
Copy link
Contributor Author

samiskin commented Sep 6, 2022

bors r+

@craig
Copy link
Contributor

craig bot commented Sep 6, 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":"At least 1 approving review is required by reviewers with write access.","documentation_url":"https://docs.github.com/articles/about-protected-branches"}

@samiskin
Copy link
Contributor Author

samiskin commented Sep 6, 2022

@stevendanna missed that this got approved last week, and now looks like I need another approval since Casper's account no longer has write access lol

@adityamaru
Copy link
Contributor

One of the failures in #86287 was also under deadlock. It might be worth skipping these heavy-weight tests under deadlock but I'm +1 on checking this in and seeing if reducing the node count brings down the execution time enough.

@samiskin
Copy link
Contributor Author

samiskin commented Sep 8, 2022

yeah we'll see how it fares 🤞

@samiskin
Copy link
Contributor Author

samiskin commented Sep 8, 2022

bors r+

@craig
Copy link
Contributor

craig bot commented Sep 8, 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.

4 participants