sql: add an example distsql tenant logic test with split ranges by rharding6373 · Pull Request #82844 · cockroachdb/cockroach

rharding6373 · 2022-06-13T19:34:03Z

This PR adds an example distsql logic test, distsql_tenant_locality,
that splits ranges and assigns them to spcific leaseholders, for future
use in testing locality-aware distsql changes.

This PR also introduces the following logic test improvements:

A new logic test config for multi-tenant multi-region testing,
3node-tenant-multiregion.
Blocklists in logic tests can now include default config lists.
Skip directives in logic tests can now include default config lists.
A default config list for multi-tenant configurations that include
3node-tenant and 3node-tenant-multi-region.
TestTenantLogic and TestTenantExecBuild now run the 3 node
tenant configuration by default and any logic/exec builder tests
that have the 3 node tenant multiregion directive.

Release note: None

cockroach-teamcity · 2022-06-13T19:34:16Z

This change is

rharding6373 · 2022-06-15T13:45:12Z

The remaining test failures relevant to this change are fixed now. Ready for a look.

yuzefovich

Reviewed 24 of 26 files at r1, 5 of 5 files at r2, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @cucaroach and @rharding6373)

pkg/sql/logictest/logic.go line 2258 at r2 (raw file):

		// config list.
		var blockedConfigNames []string
		switch blockedConfig {

nit: maybe introduce a helper function for this switch? I think we now have it in three places.

pkg/sql/logictest/logic.go line 4408 at r2 (raw file):

						!cfg.useTenant &&
						cfg.numNodes <= 5 {
						// We also cannot parallelise tests that use tenant servers

nit: Oliver wouldn't not approve of this change :D

pkg/sql/logictest/testdata/logic_test/distsql_tenant line 1 at r2 (raw file):

# LogicTest: 3node-tenant 3node-tenant-multiregion

nit: use 3node-tenant-default-configs?

pkg/sql/logictest/testdata/logic_test/distsql_tenant_locality line 35 at r2 (raw file):

user root

# TODO(harding): Once locality-aware distribution is implemented, this should

We already can execute the query in a distributed fashion, right? What's stopping us from testing that here?

pkg/sql/logictest/testdata/logic_test/distsql_tenant_locality line 39 at r2 (raw file):

# only utilize the gateway instance.
query T
EXPLAIN (DISTSQL) SELECT * FROM t WHERE v = 1 OR v = 2 OR v = 3

nit: is the goal of this test is to verify the physical planning? If so, it should go into the execbuilder tests. If - when the TODO is addressed - we want to also actually run the query, then that should stay in the main logic test folder.

rharding6373

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @cucaroach and @yuzefovich)

pkg/sql/logictest/logic.go line 2258 at r2 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

nit: maybe introduce a helper function for this switch? I think we now have it in three places.

Good idea. Cleaned this up a bit.

pkg/sql/logictest/logic.go line 4408 at r2 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

nit: Oliver wouldn't not approve of this change :D

Sigh...fine, I can be more open to alternate spellings. I've been spending lots of time lately in this file and it's been buggering (:-O) me, that's all.

In my defen[s|c]e, this is the only usage of this particular spelling outside of pkg/ui.

pkg/sql/logictest/testdata/logic_test/distsql_tenant_locality line 35 at r2 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

We already can execute the query in a distributed fashion, right? What's stopping us from testing that here?

The query currently only uses a single span, so it won't be distributed.

pkg/sql/logictest/testdata/logic_test/distsql_tenant_locality line 39 at r2 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

nit: is the goal of this test is to verify the physical planning? If so, it should go into the execbuilder tests. If - when the TODO is addressed - we want to also actually run the query, then that should stay in the main logic test folder.

I only included it as an example placeholder. The purpose of the test as a whole is to demonstrate how to set up the distribution for future testing. I removed the query altogether and just left a TODO, since it doesn't fulfill any real purpose in this PR.

yuzefovich

Reviewed 3 of 3 files at r3, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @cucaroach and @rharding6373)

pkg/sql/logictest/testdata/logic_test/distsql_tenant_locality line 39 at r2 (raw file):

Previously, rharding6373 (Rachael Harding) wrote…

I only included it as an example placeholder. The purpose of the test as a whole is to demonstrate how to set up the distribution for future testing. I removed the query altogether and just left a TODO, since it doesn't fulfill any real purpose in this PR.

Got it, thanks.

rharding6373 · 2022-06-15T22:30:51Z

TFTR!

bors r+

rharding6373 · 2022-06-15T22:48:26Z

bors r-

craig · 2022-06-15T22:48:28Z

Canceled.

rharding6373 · 2022-06-15T23:18:42Z

bors r+

craig · 2022-06-16T01:48:02Z

Build failed:

GitHub CI (Cockroach)

rharding6373 · 2022-06-16T02:31:00Z

bors r-

rharding6373 · 2022-06-16T23:03:19Z

The extra tenant logic test config was causing a test timeout in bors. The new config wasn't adding much value as a default test config, so I provided an option in the logic test infrastructure to provide a config that could be run only if it was included in a test directive.

yuzefovich

Reviewed 4 of 4 files at r4, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @cucaroach and @rharding6373)

pkg/sql/logictest/logic.go line 21 at r4 (raw file):

	"flag"
	"fmt"
	"github.com/cockroachdb/cockroach/pkg/build/bazel"

nit: the order is off.

This PR adds an example distsql logic test, `distsql_tenant_locality`, that splits ranges and assigns them to spcific leaseholders, for future use in testing locality-aware distsql changes. This PR also introduces the following logic test improvements: * A new logic test config for multi-tenant multi-region testing, `3node-tenant-multiregion`. * Blocklists in logic tests can now include default config lists. * Skip directives in logic tests can now include default config lists. * A default config list for multi-tenant configurations that include `3node-tenant` and `3node-tenant-multi-region`. * `TestTenantLogic` and `TestTenantExecBuild` now run the 3 node tenant configuration by default and any logic/exec builder tests that have the 3 node tenant multiregion directive. Release note: None

rharding6373

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

pkg/sql/logictest/logic.go line 21 at r4 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

nit: the order is off.

Fixed.

rharding6373 · 2022-06-21T15:50:15Z

TFTR!

bors r+

craig · 2022-06-21T17:28:26Z

Build succeeded:

GitHub CI (Cockroach)

rharding6373 force-pushed the distsql_tenant_testing branch 3 times, most recently from b47b90f to da266e4 Compare June 13, 2022 23:21

rharding6373 requested review from cucaroach and yuzefovich June 13, 2022 23:22

rharding6373 force-pushed the distsql_tenant_testing branch from da266e4 to 6e54e49 Compare June 13, 2022 23:30

rharding6373 marked this pull request as ready for review June 13, 2022 23:30

rharding6373 force-pushed the distsql_tenant_testing branch from 6e54e49 to 2e97c11 Compare June 15, 2022 01:50

yuzefovich reviewed Jun 15, 2022

View reviewed changes

rharding6373 force-pushed the distsql_tenant_testing branch from 2e97c11 to 6f5dcb4 Compare June 15, 2022 16:22

rharding6373 commented Jun 15, 2022

View reviewed changes

yuzefovich approved these changes Jun 15, 2022

View reviewed changes

rharding6373 force-pushed the distsql_tenant_testing branch from 6f5dcb4 to badd066 Compare June 15, 2022 20:00

rharding6373 force-pushed the distsql_tenant_testing branch from badd066 to a068d9b Compare June 16, 2022 22:59

yuzefovich approved these changes Jun 21, 2022

View reviewed changes

rharding6373 force-pushed the distsql_tenant_testing branch from a068d9b to fec5fb5 Compare June 21, 2022 15:44

rharding6373 commented Jun 21, 2022

View reviewed changes

craig bot merged commit 05e0d0d into cockroachdb:master Jun 21, 2022

Conversation

rharding6373 commented Jun 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cockroach-teamcity commented Jun 13, 2022

Uh oh!

rharding6373 commented Jun 15, 2022

Uh oh!

yuzefovich left a comment

Choose a reason for hiding this comment

Uh oh!

rharding6373 left a comment

Choose a reason for hiding this comment

Uh oh!

yuzefovich left a comment

Choose a reason for hiding this comment

Uh oh!

rharding6373 commented Jun 15, 2022

Uh oh!

rharding6373 commented Jun 15, 2022

Uh oh!

craig bot commented Jun 15, 2022

Uh oh!

rharding6373 commented Jun 15, 2022

Uh oh!

craig bot commented Jun 16, 2022

Uh oh!

rharding6373 commented Jun 16, 2022

Uh oh!

rharding6373 commented Jun 16, 2022

Uh oh!

yuzefovich left a comment

Choose a reason for hiding this comment

Uh oh!

rharding6373 left a comment

Choose a reason for hiding this comment

Uh oh!

rharding6373 commented Jun 21, 2022

Uh oh!

craig bot commented Jun 21, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

rharding6373 commented Jun 13, 2022 •

edited

Loading