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: set lower min range_max_bytes in multitenant_distsql test #99759

Merged
merged 1 commit into from
Mar 30, 2023

Conversation

rharding6373
Copy link
Collaborator

The multitenant_distsql roachtest relies on a smaller range size than the new default min of range_max_bytes (64MiB) due to performance and resource limitations. We set the COCKROACH_MIN_RANGE_MAX_BYTES to allow the test to use the smaller range.

Fixes: #99626
Epic: None

Release note: None

The multitenant_distsql roachtest relies on a smaller range size than
the new default min of range_max_bytes (64MiB) due to performance and
resource limitations. We set the COCKROACH_MIN_RANGE_MAX_BYTES to allow
the test to use the smaller range.

Fixes: cockroachdb#99626
Epic: None

Release note: None
@rharding6373 rharding6373 requested a review from a team as a code owner March 28, 2023 04:04
@rharding6373 rharding6373 requested review from srosenberg and renatolabs and removed request for a team March 28, 2023 04:04
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@mgartner
Copy link
Collaborator

@aliher1911 Does this seem sane to you? I think we need to do that same thing you did in #99636 to fix #99626.

@mgartner mgartner requested a review from aliher1911 March 28, 2023 19:46
Copy link
Collaborator Author

@rharding6373 rharding6373 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 @aliher1911, @renatolabs, and @srosenberg)


pkg/cmd/roachtest/tests/multitenant_distsql.go line 126 at r1 (raw file):

	// and test takes too long and too much resources with default range sizes
	// so make them much smaller.
	_, err = inst1Conn.Exec(`ALTER TABLE t CONFIGURE ZONE USING range_min_bytes = 1000,range_max_bytes = 100000`)

Just want to note that this is where we set the range_max_bytes in this test.

@aliher1911
Copy link
Contributor

@mgartner It worked when run roachtests manually, but I'm yet to see it merge and pass nightly. If you want to be on a safe side, we can wait for my PR to merge and run overnight before merging. But I don't see why it could fail.

@rharding6373
Copy link
Collaborator Author

This passes the zone config checks when I test with roachtest manually, as well.

@mgartner
Copy link
Collaborator

I'd vote for merging this now. In the best case, we get less noise tomorrow. In the worst case, we have to find a new fix. At least we'll have more information.

@rharding6373
Copy link
Collaborator Author

TFTR!

bors r+

@craig
Copy link
Contributor

craig bot commented Mar 28, 2023

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Mar 29, 2023

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Mar 29, 2023

Build failed (retrying...):

@rharding6373
Copy link
Collaborator Author

Will try again after flaky test fixes are in.

bors r-

@craig
Copy link
Contributor

craig bot commented Mar 29, 2023

Canceled.

@rharding6373
Copy link
Collaborator Author

bors r+

@craig craig bot merged commit 99b655d into cockroachdb:master Mar 30, 2023
@craig
Copy link
Contributor

craig bot commented Mar 30, 2023

Build succeeded:

@renatolabs
Copy link
Contributor

Sadly this doesn't seem to fix this set of roachtests. The statement to change range_max_bytes is issued against the tenant connection, and that tenant is still created without the env var, so it will still use the new range size defaults.

(The fact that we have separate code in multitenant_utils.go to start tenants replicating a lot of the logic of c.Start is also unfortunate, but I digress.)

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.

roachtest: multitenant/distsql/instances=20/bundle=off/timeout=1 failed
5 participants