bench: fix tenant benchmark by renatolabs · Pull Request #93616 · cockroachdb/cockroach

renatolabs · 2022-12-14T16:26:36Z

Fixes argument passed to crdb_internal.create_tenant to remove underscore, which is rejected as an invalid name; and also use the bench database in tenantDB.

Relates to #93530.

Epic: none
Release note: None

cockroach-teamcity · 2022-12-14T16:26:51Z

This change is

renatolabs · 2022-12-14T16:26:57Z

Example failure: https://teamcity.cockroachdb.com/viewLog.html?buildId=7959713&buildTypeId=Cockroach_Ci_Tests_Bench&tab=buildResultsDiv.

yuzefovich

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

andreimatei

Thanks for fixing this, I just noticed the failures.
Do you know if not accepting underscores is a new thing? Cause this worked yesterday...

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @renatolabs)

pkg/bench/foreachdb.go line 72 at r1 (raw file):

	// Get a SQL connection to the test tenant.
	sqlAddr := s.(*server.TestServer).TestingGetSQLAddrForTenant(ctx, tenantName)
	tenantDB := serverutils.OpenDBConn(b, sqlAddr, "bench", false, s.Stopper())

does this database exist for the tenant?

andreimatei

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @renatolabs)

pkg/bench/foreachdb.go line 60 at r1 (raw file):

	s, db, _ := serverutils.StartServer(
		b, base.TestServerArgs{
			UseDatabase:              "bench",

remove this pls. Unless it somehow affects the tenant and is the reason why the db is created in the tenant?

yuzefovich

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @andreimatei and @renatolabs)

pkg/bench/foreachdb.go line 60 at r1 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

remove this pls. Unless it somehow affects the tenant and is the reason why the db is created in the tenant?

We would need to augment all benchmarks too because queries there do hard-code bench.<table_name>.

andreimatei

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

pkg/bench/foreachdb.go line 60 at r1 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

We would need to augment all benchmarks too because queries there do hard-code bench.<table_name>.

This only affects connections returned from this node, which are no longer used by the benchmarks. The benchmarks uise the pool created below, where the "bench" db is added to the URL in this patch.
Although the benchmarks appeared to work without it in the URL too. Or perhaps I had just ran BenchmarkSQL and not others.

pkg/bench/foreachdb.go line 72 at r1 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

does this database exist for the tenant?

oh the db is created below, nvm me

yuzefovich

Reviewable status: complete! 2 of 0 LGTMs obtained (waiting on @andreimatei and @renatolabs)

pkg/bench/foreachdb.go line 60 at r1 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

This only affects connections returned from this node, which are no longer used by the benchmarks. The benchmarks uise the pool created below, where the "bench" db is added to the URL in this patch.
Although the benchmarks appeared to work without it in the URL too. Or perhaps I had just ran BenchmarkSQL and not others.

Oh, I think I misunderstood your suggestion - I thought you were saying to not use bench database and let everything run in defaultdb, nvm.

andreimatei

Do you know if not accepting underscores is a new thing? Cause this worked yesterday...

Ah, it must have been this #93269
Btw, I think there used to be a time where benchmarks were run (maybe with benchtime=1x in CI) to prevent things like this. Now this (or at least something that fails) seems to be in Bazel extended. Perhaps we could move it?

Reviewable status: complete! 2 of 0 LGTMs obtained (waiting on @renatolabs)

renatolabs · 2022-12-14T17:41:40Z

Perhaps we could move it?

There's some discussion in this link (internal).

TFTRs!

bors r=yuzefovich,andreimatei

andreimatei

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

pkg/bench/foreachdb.go line 60 at r1 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

Oh, I think I misunderstood your suggestion - I thought you were saying to not use bench database and let everything run in defaultdb, nvm.

Please remove this line; it's broken to the extent that it matters.

renatolabs · 2022-12-14T17:49:04Z

bors r-

craig · 2022-12-14T17:49:07Z

Canceled.

Fixes argument passed to `crdb_internal.create_tenant` to remove underscore, which is rejected as an invalid name; and also use the `bench` database in `tenantDB`. Relates to cockroachdb#93530. Epic: none Release note: None

renatolabs

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

pkg/bench/foreachdb.go line 60 at r1 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

Please remove this line; it's broken to the extent that it matters.

Done.

andreimatei

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

renatolabs · 2022-12-14T18:02:28Z

bors r=yuzefovich,andreimatei

craig · 2022-12-14T19:08:32Z

Build succeeded:

Bazel Essential CI (Cockroach)

renatolabs requested review from a team and andreimatei December 14, 2022 16:26

yuzefovich approved these changes Dec 14, 2022

View reviewed changes

andreimatei reviewed Dec 14, 2022

View reviewed changes

yuzefovich reviewed Dec 14, 2022

View reviewed changes

andreimatei approved these changes Dec 14, 2022

View reviewed changes

yuzefovich reviewed Dec 14, 2022

View reviewed changes

andreimatei reviewed Dec 14, 2022

View reviewed changes

renatolabs force-pushed the fix-tenant-benchmark branch from ec9bf48 to 908b95b Compare December 14, 2022 17:49

bench: fix tenant benchmark

d34ac1a

Fixes argument passed to `crdb_internal.create_tenant` to remove underscore, which is rejected as an invalid name; and also use the `bench` database in `tenantDB`. Relates to cockroachdb#93530. Epic: none Release note: None

renatolabs force-pushed the fix-tenant-benchmark branch from 908b95b to d34ac1a Compare December 14, 2022 17:50

renatolabs commented Dec 14, 2022

View reviewed changes

andreimatei approved these changes Dec 14, 2022

View reviewed changes

This was referenced Dec 14, 2022

ui: preserve backwards compatibility for managed-service #93619

Merged

ui: show regions in SQL Activity for tenants #92357

Merged

craig bot merged commit dbea195 into cockroachdb:master Dec 14, 2022

Conversation

renatolabs commented Dec 14, 2022

Uh oh!

cockroach-teamcity commented Dec 14, 2022

Uh oh!

renatolabs commented Dec 14, 2022

Uh oh!

yuzefovich left a comment

Choose a reason for hiding this comment

Uh oh!

andreimatei left a comment

Choose a reason for hiding this comment

Uh oh!

andreimatei 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!

andreimatei 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!

andreimatei left a comment

Choose a reason for hiding this comment

Uh oh!

renatolabs commented Dec 14, 2022

Uh oh!

andreimatei left a comment

Choose a reason for hiding this comment

Uh oh!

renatolabs commented Dec 14, 2022

Uh oh!

craig bot commented Dec 14, 2022

Uh oh!

renatolabs left a comment

Choose a reason for hiding this comment

Uh oh!

andreimatei left a comment

Choose a reason for hiding this comment

Uh oh!

renatolabs commented Dec 14, 2022

Uh oh!

craig bot commented Dec 14, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants