sql: remove usages of TODOTestTenantDisabled by yuzefovich · Pull Request #106883 · cockroachdb/cockroach

yuzefovich · 2023-07-16T04:40:29Z

cockroach-teamcity · 2023-07-16T04:40:40Z

This change is

knz

with a few clarifying questions

Reviewed 3 of 3 files at r1, 1 of 1 files at r2, 9 of 9 files at r3, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @stevendanna and @yuzefovich)

pkg/sql/backfill_protected_timestamp_test.go line 101 at r2 (raw file):

	}

	// By default, secondary tenants aren't allowed to mock with the zone

nit: s/mock/muck

pkg/sql/conn_executor_test.go line 1198 at r1 (raw file):

		},
	}
	tc := serverutils.StartNewTestCluster(t, 1, testClusterArgs)

maybe StartServer instead of test cluster.

pkg/sql/crdb_internal_test.go line 982 at r1 (raw file):

	ctx := context.Background()
	tc := testcluster.StartTestCluster(t, 1,

maybe StartServer with TestTenantAlwaysEnabled, and drop the StartTenant call below.

pkg/sql/run_control_test.go line 925 at r1 (raw file):

	}

	kvserver, _, _ := serverutils.StartServer(t, params)

maybe use TestTenantAlwaysEnabled and do tenant := kvserver.TenantOrServer() below.

pkg/sql/run_control_test.go line 940 at r1 (raw file):

	makeTenantConn := func() *sqlutils.SQLRunner {
		return sqlutils.MakeSQLRunner(serverutils.OpenDBConn(t, tenant.SQLAddr(), "" /* useDatabase */, false /* insecure */, kvserver.Stopper()))

that kvserver.Stopper here seems wrong, the conn should hang off the tenant stopper instead.

pkg/sql/importer/exportparquet_test.go line 294 at r3 (raw file):

	sqlDB.Exec(t, fmt.Sprintf("CREATE DATABASE %s", dbName))

	// instantiating an internal executor to easily get datums from the table

nit: capital at start of sentence and period at the end.

pkg/sql/importer/import_stmt_test.go line 138 at r3 (raw file):

	sqlDB.Exec(t, `SET CLUSTER SETTING kv.bulk_ingest.batch_size = '10KB'`)
	sqlDB.Exec(t, `SET CLUSTER SETTING storage.mvcc.range_tombstones.enabled = true`)

curious - why did you need to remove this here and below?

Three of the tests in `sql` package are explicitly managing the tenants already. Release note: None

This commit adjusts `TestValidationWithProtectedTS` to work with test tenants too. However, it seems like the test becomes noticeably slower, and improving the test speed is tracked by cockroachdb#106960. Release note: None

This commit is a grab bag of things: - remove a couple of TODOTestTenantDisabled in benchmarks to be tracked by the benchmarking issue - ensure that all tests in `sql/importer` can run with test tenant (unless explicitly disabled) - enable Export tests - adjust `TestProcessorEncountersUncertaintyError` - use correct `ExecutorConfig` in some of the tests - `TestProtectedTimestampsDuringImportInto` is marked as required separate investigation - remove updating `storage.mvcc.range_tombstones.enabled` cluster setting to `true` since it's already enabled by default on master. Most of the remaining TODOsin `sql/importer` are currently in `importer` test package (rather than `importer_test`) which would create an import cycle if `ccl` is imported, so they will be tackled separately. Release note: None

yuzefovich

TFTR!

bors r+

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

pkg/sql/conn_executor_test.go line 1198 at r1 (raw file):