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

kvcoord: Disable follower reads for TestProxyTracing #135921

Merged
merged 1 commit into from
Nov 21, 2024

Conversation

iskettaneh
Copy link
Contributor

@iskettaneh iskettaneh commented Nov 21, 2024

This commit disables follower reads for the test TestProxyTracing. We noticed that sometimes, the test gets slow, and by the time we issue a read request, it's served via follower reads instead of proxying it to the leaseholder.

Fixes: #135493

Release note: None

@iskettaneh iskettaneh requested a review from a team as a code owner November 21, 2024 17:38
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Collaborator

@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.

Reviewed all commit messages.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @iskettaneh)


-- commits line 2 at r1:
nit: "disable" -- no caps in the PR title.


pkg/kv/kvclient/kvcoord/dist_sender_server_test.go line 4877 at r1 (raw file):

		// Disable follower reads to ensure that the request is proxied, and not
		// answered locally due to follower reads.
		_, err := conn.Exec("SET CLUSTER SETTING kv.closed_timestamp.follower_reads.enabled = false")

Instead of setting the cluster setting like this, we should override it instead (like we're doing for some of the other cluster settings above). Setting it like we are here doesn't wait for cluster settings to be propagated to all nodes (this happens async), so it can lead to a test flake. We therefore prefer the Override method in these test cluster tests instead.

Copy link
Contributor Author

@iskettaneh iskettaneh left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @arulajmani)


pkg/kv/kvclient/kvcoord/dist_sender_server_test.go line 4877 at r1 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

Instead of setting the cluster setting like this, we should override it instead (like we're doing for some of the other cluster settings above). Setting it like we are here doesn't wait for cluster settings to be propagated to all nodes (this happens async), so it can lead to a test flake. We therefore prefer the Override method in these test cluster tests instead.

Done.

@iskettaneh iskettaneh force-pushed the DeflakeProxy branch 2 times, most recently from 8c36230 to 69a78dc Compare November 21, 2024 19:41
Copy link
Collaborator

@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.

:lgtm: modulo comment.

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @iskettaneh)


-- commits line 2 at r1:

Previously, arulajmani (Arul Ajmani) wrote…

nit: "disable" -- no caps in the PR title.

Did you mean to fix this?

This commit disables follower reads for the test TestProxyTracing. We
noticed that sometimes, the test gets slow, and by the time we issue a
read request, it's served via follower reads instead of proxying it to
the leaseholder.

Fixes: cockroachdb#135493

Release note: None
Copy link
Contributor Author

@iskettaneh iskettaneh left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @iskettaneh)


-- commits line 2 at r1:

Previously, arulajmani (Arul Ajmani) wrote…

Did you mean to fix this?

Yes, just changed it into Fixes: #135493

@iskettaneh iskettaneh added backport-24.1.x Flags PRs that need to be backported to 24.1. backport-24.2.x Flags PRs that need to be backported to 24.2 backport-24.3.x Flags PRs that need to be backported to 24.3 and removed backport-24.1.x Flags PRs that need to be backported to 24.1. backport-24.2.x Flags PRs that need to be backported to 24.2 backport-24.3.x Flags PRs that need to be backported to 24.3 labels Nov 21, 2024
@iskettaneh
Copy link
Contributor Author

TFTR!

bors r+

@craig craig bot merged commit a9e35d9 into cockroachdb:master Nov 21, 2024
22 of 23 checks passed
@iskettaneh
Copy link
Contributor Author

blathers backport 24.1 24.2

Copy link

blathers-crl bot commented Nov 22, 2024

Encountered an error creating backports. Some common things that can go wrong:

  1. The backport branch might have already existed.
  2. There was a merge conflict.
  3. The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.


error creating merge commit from 1b22141 to blathers/backport-release-24.1-135921: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 24.1 failed. See errors above.


error creating merge commit from 1b22141 to blathers/backport-release-24.2-135921: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 24.2 failed. See errors above.


🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.

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.

kv/kvclient/kvcoord: TestProxyTracing failed
3 participants