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

ccl/sqlproxyccl: rename TenantResolver to tenant.Directory #78143

Closed

Conversation

jaylim-crl
Copy link
Collaborator

Informs #76000.

ccl/sqlproxyccl: replace fallback resolution rule with a simple directory

Previously, --routing-rule is used as a fallback resolution if a tenant cannot
be found from the directory. Note that we no longer need this on Cloud. This
commit addresses a TODO comment by moving the resolution rule implementation
into a simple directory.

The plan is to replace the tenant.Resolver interface with a tenant.Directory
interface. The existing tenant.Directory struct will be renamed to
tenant.serviceDirectory. For now, there will be two types of directory
implementations: simple directory (a single rule), and service directory
(one that connects to a GRPC server). There are plans to add a static directory
(perhaps backed by a config file) which will enable roachprod to use sqlproxy
in a multi-tenant setup.

This TODO is also a pre-req to the load balancing work for sqlproxy. Without
that addressed, maintaining a routing rule for the balancing work can be
non-trivial.

Release note: None

ccl/sqlproxyccl: rename tenant.Directory to tenant.serviceDirectory

This commit addresses one of the TODOs to make tenant.Directory a generic
interface instead of a struct. For that to work, we'd have to rename the
existing Directory struct. This is purely a mechanical commit.

Release note: None

ccl/sqlproxyccl: add pkg/ccl/sqlproxyccl/tenant/{servicedir,simpledir}

Previously, all directory related files are located in a tenant package, which
can be confusing. This commit moves files into specific directories, and is
purely mechanical.

Release note: None

ccl/sqlproxyccl: rename tenant.Resolver to tenant.Directory

This commit completes the work of making tenant.Directory an interface.
Since we have renamed the previous Directory struct into a serviceDirectory
struct, we can now use Directory as an interface for what was previously known
as the resolver.

Release note: None

…tory

Previously, --routing-rule is used as a fallback resolution if a tenant cannot
be found from the directory. Note that we no longer need this on Cloud. This
commit addresses a TODO comment by moving the resolution rule implementation
into a simple directory.

The plan is to replace the tenant.Resolver interface with a tenant.Directory
interface. The existing tenant.Directory struct will be renamed to
tenant.serviceDirectory. For now, there will be two types of directory
implementations: simple directory (a single rule), and service directory
(one that connects to a GRPC server). There are plans to add a static directory
(perhaps backed by a config file) which will enable roachprod to use sqlproxy
in a multi-tenant setup.

This TODO is also a pre-req to the load balancing work for sqlproxy. Without
that addressed, maintaining a routing rule for the balancing work can be
non-trivial.

Release note: None
This commit addresses one of the TODOs to make tenant.Directory a generic
interface instead of a struct. For that to work, we'd have to rename the
existing Directory struct. This is purely a mechanical commit.

Release note: None
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@jaylim-crl jaylim-crl changed the title ccl/sqlproxyccl: rename tenant.Resolver to tenant.Directory ccl/sqlproxyccl: rename TenantResolver to tenant.Directory Mar 19, 2022
@jaylim-crl jaylim-crl force-pushed the jay/directory-interface branch from 77eb4e5 to a80fcae Compare March 20, 2022 00:47
Previously, all directory related files are located in a tenant package, which
can be confusing. This commit moves files into specific directories, and is
purely mechanical.

Release note: None
This commit completes the work of making tenant.Directory an interface.
Since we have renamed the previous Directory struct into a serviceDirectory
struct, we can now use Directory as an interface for what was previously known
as the resolver.

Release note: None
@jaylim-crl jaylim-crl force-pushed the jay/directory-interface branch from a80fcae to 6123205 Compare March 20, 2022 00:53
@jaylim-crl jaylim-crl marked this pull request as ready for review March 21, 2022 14:45
@jaylim-crl jaylim-crl requested review from a team as code owners March 21, 2022 14:45
@jaylim-crl jaylim-crl requested review from jeffswenson and removed request for a team March 21, 2022 14:45
@andy-kimball
Copy link
Contributor

There's an existing confusion - we have two different definitions for Directory - is it the cache of directory entries, or is it the GRPC interface that the cache uses to populate itself? It makes sense to try and resolve that, however I'm not sure the way this PR does it is best. I'm worried about changing exported package names at this stage of the release. Won't we have a tricky transition to deal with across versions of CRDB? Why not leave tenant.DirectoryClient and tenant.DirectoryServer as-is, and then introduce a new tenant.DirectoryCache interface, or maybe tenantdir.Cache if we want it in another package?

@jaylim-crl
Copy link
Collaborator Author

Discussed offline. We'll make this DirectoryCache instead.

Closing this PR in favor of #78276.

@jaylim-crl jaylim-crl closed this Mar 22, 2022
@jaylim-crl jaylim-crl deleted the jay/directory-interface branch March 22, 2022 17:48
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.

3 participants