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

roachtest: turn on DistSender circuit breakers for failover chaos tests #123820

Merged
merged 1 commit into from
May 10, 2024

Conversation

arulajmani
Copy link
Collaborator

@arulajmani arulajmani commented May 8, 2024

Failover chaos tests create asymetric partitions where DistSender circuit breakers are useful. It prevents failure modes such as #123736 (comment).

Fixes #123736

Release note: None

@arulajmani arulajmani requested a review from andrewbaptist May 8, 2024 17:03
@arulajmani arulajmani requested a review from a team as a code owner May 8, 2024 17:03
@arulajmani arulajmani requested review from herkolategan and DarrylWong and removed request for a team May 8, 2024 17:03
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@arulajmani arulajmani added the backport-24.1.x Flags PRs that need to be backported to 24.1. label May 8, 2024
Copy link
Collaborator

@andrewbaptist andrewbaptist left a comment

Choose a reason for hiding this comment

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

:lgtm:

Thanks!

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @arulajmani, @DarrylWong, and @herkolategan)


-- commits line 4 at r1:
nit: assymetric -> asymmetric


pkg/cmd/roachtest/tests/failover.go line 211 at r1 (raw file):

	rng, _ := randutil.NewTestRand()

	// Create cluster, and set up failures for all failure modes.

nit: I'm not sure you should make this change. While failers is technically not a word, this is referring to the Failer type which is used a few lines below. I think the previous wording was slightly better. I don't mind too much either way.


pkg/cmd/roachtest/tests/failover.go line 216 at r1 (raw file):

	settings.Env = append(settings.Env, "COCKROACH_SCAN_MAX_IDLE_TIME=100ms") // speed up replication

	// DistSender circuit breakers are useful for these chaos tests. Turn them on.

nit: consider adding a TODO either here or in the definition of the variable to remove this if we later make this the default.

Failover chaos tests create asymetric partitions where DistSender
circuit breakers are useful. It prevents failure modes such as
cockroachdb#123736 (comment).

Fixes cockroachdb#123736

Release note: None
Copy link
Collaborator Author

@arulajmani arulajmani left a comment

Choose a reason for hiding this comment

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

TFTRs!

bors r=nicktrav,andrewbaptist

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @andrewbaptist, @DarrylWong, and @herkolategan)


-- commits line 4 at r1:

Previously, andrewbaptist (Andrew Baptist) wrote…

nit: assymetric -> asymmetric

Done.


pkg/cmd/roachtest/tests/failover.go line 211 at r1 (raw file):

Previously, andrewbaptist (Andrew Baptist) wrote…

nit: I'm not sure you should make this change. While failers is technically not a word, this is referring to the Failer type which is used a few lines below. I think the previous wording was slightly better. I don't mind too much either way.

I see, I thought it was a typo. I've reverted.


pkg/cmd/roachtest/tests/failover.go line 216 at r1 (raw file):

Previously, andrewbaptist (Andrew Baptist) wrote…

nit: consider adding a TODO either here or in the definition of the variable to remove this if we later make this the default.

Done.

@craig
Copy link
Contributor

craig bot commented May 8, 2024

Build failed:

@arulajmani
Copy link
Collaborator Author

bors retry

@craig craig bot merged commit 6e1b82d into cockroachdb:master May 10, 2024
18 of 22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-24.1.x Flags PRs that need to be backported to 24.1.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

roachtest: failover/chaos/read-only failed
4 participants