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

workload: add follower read support #113094

Merged

Conversation

andrewbaptist
Copy link
Collaborator

@andrewbaptist andrewbaptist commented Oct 25, 2023

The performance of the system can be different if there are reads as of a system time. This PR adds the ability to issue follower reads.

Epic: none

Release note (cli change): Add --follower-read-percent to the workload kv run.

@andrewbaptist andrewbaptist requested a review from a team as a code owner October 25, 2023 21:12
@andrewbaptist andrewbaptist requested review from herkolategan and srosenberg and removed request for a team October 25, 2023 21:12
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@andrewbaptist
Copy link
Collaborator Author

This is part of the testing effort for #112351. It allows running follower reads for KV workloads.

@andrewbaptist andrewbaptist requested review from a team as code owners October 26, 2023 13:24
@andrewbaptist andrewbaptist requested review from msbutler and removed request for a team October 26, 2023 13:24
@andrewbaptist andrewbaptist force-pushed the 2023-10-25-follower-read-for-kv branch 2 times, most recently from b36816f to 0901a74 Compare October 26, 2023 13:29
@andrewbaptist
Copy link
Collaborator Author

andrewbaptist commented Oct 26, 2023

@kvoli This can't merge yet as this depends on the fix for follower reads (otherwise this test will fail). However, I wanted to have you review these changes as they are in a test you have been mostly involved in.

Copy link
Member

@nvanbenschoten nvanbenschoten left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @andrewbaptist, @herkolategan, @kvoli, @msbutler, and @srosenberg)


pkg/cmd/roachtest/tests/kv.go line 542 at r2 (raw file):

			// Don't connect to the node we are going to shut down.
			dbs := make([]*gosql.DB, nodes-1)
			for i := 0; i < nodes-1; i++ {

nit: for i := range dbs {


pkg/cmd/roachtest/tests/kv.go line 615 at r2 (raw file):

			t.Status("gracefully draining and restarting nodes")
			// Gracefully shut down the third node, let the cluster run for a
			// while, then restart it. Then repeat for good measure.

s/Then repeat/Repeat for a total of 3 times/

To make it more clear where the 3 is coming from below.


pkg/cmd/roachtest/tests/kv.go line 637 at r2 (raw file):

				// drop all packets in both directions.
				if !c.IsLocal() {
					c.Run(ctx, c.Node(nodes), `sudo iptables -A INPUT -p tcp --dport 26257 -j DROP`)

Nice idea.


pkg/workload/kv/kv.go line 99 at r1 (raw file):

	insertCount                          int
	useBackgroundTxnQoS                  bool
	followerReadPercent                  int

nit: move this right below readPercent.


pkg/workload/kv/kv.go line 185 at r1 (raw file):

		g.flags.BoolVar(&g.useBackgroundTxnQoS, `background-qos`, false,
			`Set default_transaction_quality_of_service session variable to "background".`)
		g.flags.IntVar(&g.followerReadPercent, `follower-read-percent`, 0,

nit: move this below g.flags.IntVar(&g.readPercent...


pkg/workload/kv/kv.go line 185 at r1 (raw file):

		g.flags.BoolVar(&g.useBackgroundTxnQoS, `background-qos`, false,
			`Set default_transaction_quality_of_service session variable to "background".`)
		g.flags.IntVar(&g.followerReadPercent, `follower-read-percent`, 0,

It was previously possible to accomplish this by adding the default_transaction_use_follower_reads session variable to the db URL that you pass to KV, but this is much nicer and also allows for a fraction of the read to be from followers.


pkg/workload/kv/kv.go line 601 at r1 (raw file):

		readStmt := o.readStmt

		if o.config.followerReadPercent > 0 &&

nit: do we need the o.config.followerReadPercent > 0 condition?

Copy link
Collaborator Author

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

TFTR - I updated the code, but am only going to merge the first commit (change workload to support follower reads) here once approved, and then move the other commit to the branch which has the fix for follower read stopping.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @herkolategan, @kvoli, @msbutler, @nvanbenschoten, and @srosenberg)


pkg/cmd/roachtest/tests/kv.go line 542 at r2 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

nit: for i := range dbs {

Done


pkg/cmd/roachtest/tests/kv.go line 615 at r2 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

s/Then repeat/Repeat for a total of 3 times/

To make it more clear where the 3 is coming from below.

Done


pkg/cmd/roachtest/tests/kv.go line 637 at r2 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Nice idea.

Thanks - I realized it wasn't reproducing reliably with "clean" shutdown.


pkg/workload/kv/kv.go line 99 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

nit: move this right below readPercent.

Done


pkg/workload/kv/kv.go line 185 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

nit: move this below g.flags.IntVar(&g.readPercent...

Done


pkg/workload/kv/kv.go line 185 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

It was previously possible to accomplish this by adding the default_transaction_use_follower_reads session variable to the db URL that you pass to KV, but this is much nicer and also allows for a fraction of the read to be from followers.

Right - I had considered that previously, but I wanted to make sure that the test at least covered both types of reads since the cause for slowdowns might have been different for each.


pkg/workload/kv/kv.go line 601 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

nit: do we need the o.config.followerReadPercent > 0 condition?

No - removed to make it more clear.

Copy link
Collaborator

@kvoli kvoli left a comment

Choose a reason for hiding this comment

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

First commit :lgtm:

Reviewed 2 of 2 files at r3.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @andrewbaptist, @herkolategan, @msbutler, @nvanbenschoten, and @srosenberg)


-- commits line 5 at r1:
nit: could you mention adding follower read support for the kv workload specifically.

The performance of the system can be different if there are reads as of
a system time. This PR adds the ability to issue follower reads.

Epic: none

Release note (cli change): Add --follower-read-percent to the workload kv run.
@andrewbaptist
Copy link
Collaborator Author

bors r=kvoli

TFTR!
I pulled off the second commit so I can add the changes to the workload tool first prior to the follower read changes being made. I moved the second commit to: #112926.

@craig
Copy link
Contributor

craig bot commented Nov 2, 2023

Build succeeded:

@craig craig bot merged commit 128e987 into cockroachdb:master Nov 2, 2023
7 of 8 checks passed
@andrewbaptist andrewbaptist deleted the 2023-10-25-follower-read-for-kv branch November 2, 2023 20:18
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.

None yet

4 participants