Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

pkg/server: move DataDistribution to systemAdminServer #100620

Merged
merged 1 commit into from
Apr 7, 2023

Conversation

abarganier
Copy link
Contributor

The DataDistribution endpoint reports replica counts by database and table. When it was built, it operated off the assumption that a range would only ever contain a single table's data within.

Now that we have coalesced ranges, a single range can span multiple tables. Unfortunately, the DataDistribution endpoint does not take this fact into account, meaning it reports garbled and inaccurate data, unless the spanconfig.storage_coalesce_adjacent.enabled setting is set to false (see #98820).

For secondary tenants, ranges are always coalesced, so this endpoint in its current state could never report meaningful data for a tenant.

Given all of this, we have decided to make this endpoint only available for the system tenant. This patch
accomplishes this by moving the endpoint away from the adminServer and into the systemAdminServer, making it effectively unimplemented for secondary tenants.

Release note: none

Informs: #97942

The DataDistribution endpoint reports replica counts by
database and table. When it was built, it operated off
the assumption that a range would only ever contain a
single table's data within.

Now that we have coalesced ranges, a single range can
span multiple tables. Unfortunately, the DataDistribution
endpoint does not take this fact into account, meaning
it reports garbled and inaccurate data, unless the
`spanconfig.storage_coalesce_adjacent.enabled` setting
is set to false (see cockroachdb#98820).

For secondary tenants, ranges are *always* coalesced,
so this endpoint in its current state could never report
meaningful data for a tenant.

Given all of this, we have decided to make this endpoint
only available for the system tenant. This patch
accomplishes this by moving the endpoint away from the
adminServer and into the systemAdminServer, making
it effectively unimplemented for secondary tenants.

Release note: none
@abarganier abarganier requested review from dhartunian, irfansharif and a team April 4, 2023 18:30
@abarganier abarganier requested a review from a team as a code owner April 4, 2023 18:30
@blathers-crl
Copy link

blathers-crl bot commented Apr 4, 2023

It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR?

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.

@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Collaborator

@dhartunian dhartunian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

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

@abarganier
Copy link
Contributor Author

TFTR!

bors r=dhartunian

@rickystewart
Copy link
Collaborator

bors r=dhartunian

@craig
Copy link
Contributor

craig bot commented Apr 7, 2023

Build succeeded:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants