multitenant: make the system tenant appear to have all capabilities#105146
multitenant: make the system tenant appear to have all capabilities#105146craig[bot] merged 2 commits intocockroachdb:masterfrom
Conversation
|
This change is |
1d92fcc to
7e0fd34
Compare
yuzefovich
left a comment
There was a problem hiding this comment.
I only have a couple of questions for my own edification.
Reviewed 4 of 4 files at r1, 5 of 5 files at r2, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @knz and @stevendanna)
pkg/multitenant/tenantcapabilities/capabilities.go line 21 at r2 (raw file):
// ID represents a handle to a tenant capability. type ID uint8
Just curious what was the rationale for choosing uint8 here. It seems feasible (although improbable) that we'll have more than 256 capabilities in the future. Is the size of ID important optimization? Is there an RFC or some PR I could read about it?
pkg/ccl/logictestccl/testdata/logic_test/tenant_capability line 221 at r2 (raw file):
subtest regression_98749 query TT colnames
nit: I wonder whether we should add rowsort option or ORDER BY to the query to ensure deterministic output.
pkg/multitenant/tenantcapabilities/values_test.go line 34 at r1 (raw file):
} func TestGetSet(t *testing.T) {
Do you know what the purpose of this test is? That it doesn't panic?
7e0fd34 to
d4a8293
Compare
knz
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/multitenant/tenantcapabilities/capabilities.go line 21 at r2 (raw file):
Previously, yuzefovich (Yahor Yuzefovich) wrote…
Just curious what was the rationale for choosing uint8 here. It seems feasible (although improbable) that we'll have more than 256 capabilities in the future. Is the size of
IDimportant optimization? Is there an RFC or some PR I could read about it?
It's a general principle I learned from Radu: use the smallest type large enough for an enum. It ensures that structs that embed a field of this type don't get unnecessary large.
For further discussion, I would just ping folk on #go-learners.
NB that if the type ever becomes "too small" we'd immediately become alerted by unit tests.
pkg/ccl/logictestccl/testdata/logic_test/tenant_capability line 221 at r2 (raw file):
Previously, yuzefovich (Yahor Yuzefovich) wrote…
nit: I wonder whether we should add
rowsortoption or ORDER BY to the query to ensure deterministic output.
let's do it. Done.
pkg/multitenant/tenantcapabilities/values_test.go line 34 at r1 (raw file):
Previously, yuzefovich (Yahor Yuzefovich) wrote…
Do you know what the purpose of this test is? That it doesn't panic?
It's to ensure the default value has a representation and is settable. Added a comment to clarify.
|
bors r=yuzefovich |
|
Build failed (retrying...): |
|
Build failed: |
|
bors r=yuzefovich |
d4a8293 to
c59c04b
Compare
|
Canceled. |
|
bors r=yuzefovich |
|
Build failed: |
This struct was ill-defined: what does it mean for a cap struct to be "default"? Default relative to what? There are only two things that matter: - the empty protobuf, which is the actual implementation default. - whichever set of capabilities is taken over from a template in CREATE TENANT ... LIKE. In fact, `DefaultCapabilities` so far was only used in tests. This commit removes it, to avoid any confusion. Release note: None
The system tenant is currently defined to have access to all services. Yet, the output of `SHOW TENANT system WITH CAPABILITIES` suggested that was not true. This patch fixes that. Release note: None
c59c04b to
7ab3e49
Compare
|
bors r=yuzefovich |
|
Build succeeded: |
|
Encountered an error creating backports. Some common things that can go wrong:
You might need to create your backport manually using the backport tool. error creating merge commit from 7ab3e49 to blathers/backport-release-23.1-105146: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict [] you may need to manually resolve merge conflicts with the backport tool. Backport to branch 23.1.x failed. See errors above. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
Epic: CRDB-26691
Fixes #98749.
The system tenant is currently defined to have access to all services.
Yet, the output of
SHOW TENANT system WITH CAPABILITIESsuggestedthat was not true.
This patch fixes that.