multitenant: move `tenantcapabilities.IDs` to `tenantcapabilitiespb` #141774

shubhamdhama · 2025-02-20T10:13:31Z

multitenant: relocate `tenantcapabilitiespb` directory

This commit relocates the tenantcapabilitiespb directory to the same
level as tenantcapabilities. This change improves consistency with other
similar directories (e.g., mtinfopb) and shortens the long package path
(pkg/multitenant/tenantcapabilities).

multitenant: move `tenantcapabilities.IDs` to `tenantcapabilitiespb`

This commit moves tenantcapabilities.IDs from
pkg/multitenant/tenantcapabilities to tenantcapabilitiespb to resolve a
cyclic dependency. pkg/base needs to use capability constants/enums, but
a direct dependency on pkg/multitenant/tenantcapabilities would create a
cycle. tenantcapabilitiespb has fewer dependencies, making it a more
suitable location for these IDs. It also makes sense to keep high-level
contracts like these in the tenantcapabilitiespb package.

Informs: #138912
Epic: CRDB-38970
Release note: None

cockroach-teamcity · 2025-02-20T10:14:00Z

This change is

rafiss

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andyyang890, @angles-n-daemons, @DarrylWong, @golgeek, @kev-cao, and @yuzefovich)

pkg/multitenant/tenantcapabilities/capability.go line 95 at r3 (raw file):

// EnableAll enables maximum access to services.
func EnableAll(t *tenantcapabilitiespb.TenantCapabilities) {

is this function for tests only? if so, let's name it TestingEnableAll to make that clear.

we use that convention a lot, for example:

cockroach/pkg/ccl/changefeedccl/changefeedbase/settings.go

Lines 33 to 39 in 64f47b4

    
           // TestingSetDefaultMinCheckpointFrequency changes DefaultMinCheckpointFrequency for tests. 
        
           // Returns function to restore flush frequency to its original value. 
        
           func TestingSetDefaultMinCheckpointFrequency(f time.Duration) func() { 
        
           	old := DefaultMinCheckpointFrequency 
        
           	DefaultMinCheckpointFrequency = f 
        
           	return func() { DefaultMinCheckpointFrequency = old } 
        
           }

(i haven't reviewd the PR closely yet, but the same naming convention should be used for any other testing functions that were introduced here)

rafiss

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andyyang890, @angles-n-daemons, @DarrylWong, @golgeek, @kev-cao, @shubhamdhama, and @yuzefovich)

pkg/multitenant/tenantcapabilities/capability.go line 95 at r3 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

is this function for tests only? if so, let's name it TestingEnableAll to make that clear.

we use that convention a lot, for example:

cockroach/pkg/ccl/changefeedccl/changefeedbase/settings.go

Lines 33 to 39 in 64f47b4

// TestingSetDefaultMinCheckpointFrequency changes DefaultMinCheckpointFrequency for tests.

// Returns function to restore flush frequency to its original value.

func TestingSetDefaultMinCheckpointFrequency(f time.Duration) func() {

old := DefaultMinCheckpointFrequency

DefaultMinCheckpointFrequency = f

return func() { DefaultMinCheckpointFrequency = old }

}

(i haven't reviewd the PR closely yet, but the same naming convention should be used for any other testing functions that were introduced here)

and also, similar to that example, it's prudent to have these testing functions return a cleanup function that resets everything back to the previous state, so that cleanup can be called by a defer statement.

shubhamdhama · 2025-02-20T16:06:00Z

pkg/multitenant/tenantcapabilities/capability.go line 95 at r3 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

and also, similar to that example, it's prudent to have these testing functions return a cleanup function that resets everything back to the previous state, so that cleanup can be called by a defer statement.

This is just a code relocation: some existing code from capabilities.go has been moved to capability.go. No new functionality has been added.

pkg/backup/backup_test.go

github-actions · 2025-02-21T13:18:22Z

🟢 Sysbench [SQL, 3node, oltp_read_write]

Metric	Old Commit	New Commit	Delta	Note	Threshold
🟢 sec/op	10.99m ±1%	10.93m ±0%	-0.52%	p=0.035 n=10	3.0%
⚪ errs/op	0.000 ±0%	0.000 ±0%	~	p=1.000 n=10	0.0%
⚪ allocs/op	10.15k ±1%	10.17k ±0%	~	p=0.469 n=10	2.0%
⚪ B/op	2.204Mi ±0%	2.205Mi ±0%	~	p=0.853 n=10	2.0%

Reproduce

benchdiff binaries:

mkdir -p benchdiff/9c5d352/bin/1058449141
gcloud storage cp gs://cockroach-microbench-ci/builds/9c5d35278925ab275bc3030de676cbd8dc0ee56d/bin/pkg_sql_tests benchdiff/9c5d352/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
chmod +x benchdiff/9c5d352/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
mkdir -p benchdiff/fbcabe0/bin/1058449141
gcloud storage cp gs://cockroach-microbench-ci/builds/fbcabe029eb2e1701710d08cebbca6d15089f911/bin/pkg_sql_tests benchdiff/fbcabe0/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
chmod +x benchdiff/fbcabe0/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests

benchdiff command:

benchdiff --run=^BenchmarkSysbench/SQL/3node/oltp_read_write$ --old=fbcabe0 --new=9c5d352 ./pkg/sql/tests

🟢 Sysbench [KV, 1node, local, oltp_read_only]

Metric	Old Commit	New Commit	Delta	Note	Threshold
🟢 sec/op	666.7µ ±1%	658.4µ ±1%	-1.26%	p=0.015 n=10	2.0%
⚪ errs/op	0.000 ±0%	0.000 ±0%	~	p=1.000 n=10	0.0%
⚪ allocs/op	439.0 ±0%	439.0 ±0%	~	p=1.000 n=10	1.5%
🟢 B/op	254.2Ki ±0%	254.1Ki ±0%	-0.02%	p=0.006 n=10	1.5%

Reproduce

benchdiff binaries:

mkdir -p benchdiff/9c5d352/bin/1058449141
gcloud storage cp gs://cockroach-microbench-ci/builds/9c5d35278925ab275bc3030de676cbd8dc0ee56d/bin/pkg_sql_tests benchdiff/9c5d352/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
chmod +x benchdiff/9c5d352/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
mkdir -p benchdiff/fbcabe0/bin/1058449141
gcloud storage cp gs://cockroach-microbench-ci/builds/fbcabe029eb2e1701710d08cebbca6d15089f911/bin/pkg_sql_tests benchdiff/fbcabe0/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
chmod +x benchdiff/fbcabe0/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests

benchdiff command:

benchdiff --run=^BenchmarkSysbench/KV/1node_local/oltp_read_only$ --old=fbcabe0 --new=9c5d352 ./pkg/sql/tests

⚪ Sysbench [KV, 1node, local, oltp_write_only]

Metric	Old Commit	New Commit	Delta	Note	Threshold
⚪ sec/op	1.303m ±2%	1.301m ±3%	~	p=0.280 n=10	2.5%
⚪ errs/op	0.000 ±0%	0.000 ±0%	~	p=1.000 n=10	0.0%
⚪ allocs/op	1.391k ±0%	1.390k ±0%	~	p=0.548 n=10	1.8%
⚪ B/op	288.7Ki ±0%	288.7Ki ±1%	~	p=0.684 n=10	1.8%

Reproduce

benchdiff binaries:

mkdir -p benchdiff/9c5d352/bin/1058449141
gcloud storage cp gs://cockroach-microbench-ci/builds/9c5d35278925ab275bc3030de676cbd8dc0ee56d/bin/pkg_sql_tests benchdiff/9c5d352/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
chmod +x benchdiff/9c5d352/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
mkdir -p benchdiff/fbcabe0/bin/1058449141
gcloud storage cp gs://cockroach-microbench-ci/builds/fbcabe029eb2e1701710d08cebbca6d15089f911/bin/pkg_sql_tests benchdiff/fbcabe0/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
chmod +x benchdiff/fbcabe0/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests

benchdiff command:

benchdiff --run=^BenchmarkSysbench/KV/1node_local/oltp_write_only$ --old=fbcabe0 --new=9c5d352 ./pkg/sql/tests

Artifacts

download:

mkdir -p new
gcloud storage cp gs://cockroach-microbench-ci/artifacts/9c5d35278925ab275bc3030de676cbd8dc0ee56d/13457216717-1/\* new/
mkdir -p old
gcloud storage cp gs://cockroach-microbench-ci/artifacts/fbcabe029eb2e1701710d08cebbca6d15089f911/13457216717-1/\* old/

Legend

⚪ Neutral: No significant performance change.
🟡 Warning: Slight degradation, likely due to variance, but still within thresholds.
🔴 Regression: Likely performance regression, requiring investigation.
🟢 Improvement: Possible performance gain.

No regressions detected!

built with commit: 9c5d35278925ab275bc3030de676cbd8dc0ee56d

pkg/multitenant/tenantcapabilities/capability.go

This commit relocates the `tenantcapabilitiespb` directory to the same level as `tenantcapabilities`. This change improves consistency with other similar directories (e.g., `mtinfopb`) and shortens the long package path (`pkg/multitenant/tenantcapabilities`).

This commit moves `tenantcapabilities.IDs` from `pkg/multitenant/tenantcapabilities` to `tenantcapabilitiespb` to resolve a cyclic dependency. `pkg/base` needs to use capability constants/enums, but a direct dependency on `pkg/multitenant/tenantcapabilities` would create a cycle. `tenantcapabilitiespb` has fewer dependencies, making it a more suitable location for these IDs. It also makes sense to keep high-level contracts like these in the `tenantcapabilitiespb` package. Informs: cockroachdb#138912 Epic: CRDB-38970 Release note: None

cthumuluru-crdb

Thank you. LGTM!

shubhamdhama · 2025-03-06T09:48:30Z

Thanks for the reviews!

bors r=stevendanna,cthumuluru-crdb

shubhamdhama · 2025-03-06T09:59:02Z

pkg/multitenant/tenantcapabilities/capability.go line 95 at r3 (raw file):

Previously, shubhamdhama (Shubham Dhama) wrote…

This is just a code relocation: some existing code from capabilities.go has been moved to capability.go. No new functionality has been added.

@rafiss, it's actually used for non-test code, so I think we are good here:

cockroach/pkg/multitenant/mtinfo/info.go

Lines 69 to 71 in c68c559

    
           if tid.IsSystem() { 
        
           	tenantcapabilities.EnableAll(&info.ProtoInfo.Capabilities) 
        
           }

craig · 2025-03-06T10:28:55Z

Build succeeded:

shubhamdhama requested review from a team as code owners February 20, 2025 10:13

shubhamdhama requested review from kev-cao, andyyang890, golgeek, DarrylWong, angles-n-daemons and yuzefovich and removed request for a team February 20, 2025 10:13

rafiss reviewed Feb 20, 2025

View reviewed changes

shubhamdhama requested review from stevendanna and cthumuluru-crdb February 21, 2025 08:25

stevendanna approved these changes Feb 21, 2025

View reviewed changes

shubhamdhama force-pushed the refactor-tenant-capabilities branch from 5373bc4 to 9c5d352 Compare February 21, 2025 12:53

shubhamdhama commented Feb 21, 2025

View reviewed changes

pkg/backup/backup_test.go Outdated Show resolved Hide resolved

shubhamdhama force-pushed the refactor-tenant-capabilities branch 3 times, most recently from 5b91044 to 226b6e6 Compare March 4, 2025 08:23

cthumuluru-crdb requested changes Mar 4, 2025

View reviewed changes

pkg/multitenant/tenantcapabilities/capability.go Show resolved Hide resolved

shubhamdhama added 2 commits March 6, 2025 11:29

shubhamdhama force-pushed the refactor-tenant-capabilities branch from 226b6e6 to ecca7f5 Compare March 6, 2025 06:13

shubhamdhama requested a review from cthumuluru-crdb March 6, 2025 09:40

cthumuluru-crdb approved these changes Mar 6, 2025

View reviewed changes

craig bot merged commit f54cc8e into cockroachdb:master Mar 6, 2025
24 checks passed

celeste-cockroachdb bot added the target-release-25.2.0 label Mar 6, 2025

celeste-cockroachdb bot added v25.2.0-prerelease and removed target-release-25.2.0 labels Mar 24, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

multitenant: move `tenantcapabilities.IDs` to `tenantcapabilitiespb` #141774

multitenant: move `tenantcapabilities.IDs` to `tenantcapabilitiespb` #141774

shubhamdhama commented Feb 20, 2025 •

edited

Loading

cockroach-teamcity commented Feb 20, 2025

rafiss left a comment

rafiss left a comment

shubhamdhama commented Feb 20, 2025

github-actions bot commented Feb 21, 2025

cthumuluru-crdb left a comment

shubhamdhama commented Mar 6, 2025

shubhamdhama commented Mar 6, 2025 •

edited

Loading

craig bot commented Mar 6, 2025

	// TestingSetDefaultMinCheckpointFrequency changes DefaultMinCheckpointFrequency for tests.
	// Returns function to restore flush frequency to its original value.
	func TestingSetDefaultMinCheckpointFrequency(f time.Duration) func() {
	old := DefaultMinCheckpointFrequency
	DefaultMinCheckpointFrequency = f
	return func() { DefaultMinCheckpointFrequency = old }
	}

multitenant: move tenantcapabilities.IDs to tenantcapabilitiespb #141774

multitenant: move tenantcapabilities.IDs to tenantcapabilitiespb #141774

Conversation

shubhamdhama commented Feb 20, 2025 • edited Loading

cockroach-teamcity commented Feb 20, 2025

rafiss left a comment

Choose a reason for hiding this comment

rafiss left a comment

Choose a reason for hiding this comment

shubhamdhama commented Feb 20, 2025

github-actions bot commented Feb 21, 2025

cthumuluru-crdb left a comment

Choose a reason for hiding this comment

shubhamdhama commented Mar 6, 2025

shubhamdhama commented Mar 6, 2025 • edited Loading

craig bot commented Mar 6, 2025

multitenant: move `tenantcapabilities.IDs` to `tenantcapabilitiespb` #141774

multitenant: move `tenantcapabilities.IDs` to `tenantcapabilitiespb` #141774

shubhamdhama commented Feb 20, 2025 •

edited

Loading

shubhamdhama commented Mar 6, 2025 •

edited

Loading