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.

Sign up for GitHub

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

Jump to bottom

sql: support crdb_internal.ranges{_no_leases} for secondary tenants #92029

Closed
wants to merge 4 commits into from
Closed

sql: support crdb_internal.ranges{_no_leases} for secondary tenants #92029

wants to merge 4 commits into from

Conversation

aayushshah15
Copy link
Contributor

@aayushshah15 aayushshah15 commented Nov 17, 2022 •
edited
Loading

This PR makes crdb_internal.ranges{_no_leases} and SHOW RANGES
work for secondary tenants. It achieves this by leveraging the
DistSender on the secondary tenant to perform privileged meta2
lookups over the tenant Connector boundary.

Release note (sql change): crdb_internal.ranges{_no_leases} and
SHOW RANGES statements now work on secondary tenants.

Epic: https://cockroachlabs.atlassian.net/browse/CRDB-14522

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@aayushshah15 aayushshah15 force-pushed the readAllRanges branch 2 times, most recently from 964a5df to 2548610 Compare November 17, 2022 02:51
@aayushshah15 aayushshah15 force-pushed the readAllRanges branch 2 times, most recently from 754f3e3 to 4eb5fca Compare November 17, 2022 05:08
knz
knz reviewed Nov 17, 2022
View reviewed changes
Copy link
Contributor

@knz knz left a comment

Choose a reason for hiding this comment

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

This is good.
LGTM with nits.

Reviewed 2 of 2 files at r1, 11 of 11 files at r2, 1 of 1 files at r3, 2 of 2 files at r4, all commit messages.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @aayushshah15)

-- commits line 2 at r1:
No objection to this fix, but does it belong to this PR?

pkg/sql/crdb_internal.go line 3668 at r4 (raw file):

				return nil, nil
			}
			defer ri.Next(ctx)

This is elegant but it feels like magic. Please add a comment above the function definition that explains how the iteration works.

pkg/ccl/logictestccl/testdata/logic_test/crdb_internal_tenant line 307 at r4 (raw file):

SELECT range_id, start_pretty, end_pretty, lease_holder FROM crdb_internal.ranges

53  /Tenant/10  /Max  1

I appreciate this is not directly a result of your change, but having /Max as end key is problematic.

What would it take for this to be /Tenant/10/<endprefix> or something similar? If it's a simple change in your PR I'll take it, otherwise please file an issue so we can track this.

@knz
Copy link
Contributor

knz commented Nov 17, 2022

@ecwall would you like to review this PR as well? I believe there are a few things in there you'll find interesting to learn.

@aayushshah15 aayushshah15 marked this pull request as ready for review November 17, 2022 15:12
@aayushshah15 aayushshah15 requested review from a team as code owners November 17, 2022 15:12
@aayushshah15 aayushshah15 requested a review from a team November 17, 2022 15:12
@aayushshah15 aayushshah15 requested a review from a team as a code owner November 17, 2022 15:12
@aayushshah15 aayushshah15 requested review from a team, herkolategan and smg260 and removed request for a team November 17, 2022 15:12
aayushshah15
aayushshah15 commented Nov 17, 2022
View reviewed changes
Copy link
Contributor Author

@aayushshah15 aayushshah15 left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @herkolategan, @knz, and @smg260)

-- commits line 2 at r1:

Previously, knz (Raphael 'kena' Poss) wrote…

No objection to this fix, but does it belong to this PR?

cockroach demo stalls without this fix since it relies on this logic during startup.

pkg/sql/crdb_internal.go line 3668 at r4 (raw file):

Previously, knz (Raphael 'kena' Poss) wrote…

This is elegant but it feels like magic. Please add a comment above the function definition that explains how the iteration works.

Let me know if that comment doesn't quite cover it.

pkg/ccl/logictestccl/testdata/logic_test/crdb_internal_tenant line 307 at r4 (raw file):

Previously, knz (Raphael 'kena' Poss) wrote…

I appreciate this is not directly a result of your change, but having /Max as end key is problematic.

What would it take for this to be /Tenant/10/<endprefix> or something similar? If it's a simple change in your PR I'll take it, otherwise please file an issue so we can track this.

Filed: #92072

@aayushshah15 aayushshah15 force-pushed the readAllRanges branch 2 times, most recently from 4494baa to d55977d Compare November 17, 2022 18:01
@aayushshah15
workload: fix stall in workload splits under multi-tenancy
a7d4d18 
This commit fixes a stall when running `workload` splits under a secondary
tenant. Secondary tenants don't support splits or scatter (as of the time of
this commit), and `workload`'s splitting logic was buggy and would stall under
multi-tenancy.

Release note: None
@aayushshah15
kvtenant: expose node localities through the tenant connector
7d81e0e 
This commit exposes an interface through the tenant connector that provides a
map of node localities from the host cluster. This will be used in a future
commit to power `crdb_internal.ranges{_no_leases}`.

Release note: None
@aayushshah15
rpc: allow LeaseInfo and RangeStats requests for secondary tenants
b1a8dff 
This commit makes it such that secondary tenants are allowed to issue
`LeaseInfo` and `RangeStats` requests. A future commit will need this to
allow `crdb_internal.ranges{_no_leases}` to work on tenants.

Release note: None
@aayushshah15
sql: support crdb_internal.ranges{_no_leases} for secondary tenants
00635d8 
This commit makes `crdb_internal.ranges{_no_leases}` and `SHOW RANGES`
work for secondary tenants. It achieves this by leveraging the
`DistSender` on the secondary tenant to perform privileged meta2
lookups over the tenant `Connector` boundary.

Release note (sql change): `crdb_internal.ranges{_no_leases}` and `SHOW
RANGES` statements now work on secondary tenants.

Epic: https://cockroachlabs.atlassian.net/browse/CRDB-14522
@blathers-crl
Copy link

blathers-crl bot commented Nov 17, 2022

Thank you for updating your pull request.

Before a member of our team reviews your PR, I have some potential action items for you:

  • We notice you have more than one commit in your PR. We try break logical changes into separate commits, but commits such as "fix typo" or "address review commits" should be squashed into one commit and pushed with --force
  • When CI has completed, please ensure no errors have appeared.

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.

@blathers-crl blathers-crl bot added the O-community Originated from the community label Nov 17, 2022
@ecwall
Copy link
Contributor

ecwall commented Nov 18, 2022

Moving work to #92131

@ecwall ecwall closed this Nov 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@knz knz knz left review comments

@herkolategan herkolategan Awaiting requested review from herkolategan herkolategan is a code owner automatically assigned from cockroachdb/test-eng

@smg260 smg260 Awaiting requested review from smg260 smg260 is a code owner automatically assigned from cockroachdb/test-eng

Assignees
No one assigned
Labels
O-community Originated from the community
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@aayushshah15 @cockroach-teamcity @knz @ecwall