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

base: introduce SQLInstanceID (and container) #48110

Merged
merged 12 commits into from
May 1, 2020
Merged

Conversation

tbg
Copy link
Member

@tbg tbg commented Apr 28, 2020

With the advent of multi-tenancy, SQL servers can optionally be
decoupled from the (and share a) KV backend. As a consequence, the
NodeID may not be available and its use across all components that need
to function in a multi-tenant setup is deprecated. Yet, some components
require a similar unique identifier for the SQL server they're a part
of.

This new identifier is the SQLInstanceID. At the time of writing,
since we're not yet able to start true SQL tenant servers and only
exercise parts of this functionality in unit testing, it is a
randomized integer. It's possible that we'll replace it with
a per-tenant counter in production, putting it truly on par with
the NodeID, to be able to give the simple guarantee that these IDs
are never reused between different incarnations of the SQL process
for a given tenant. (Note that initially, at most one SQL tenant
server will be active per tenant at any given time).

This commit introduces an IDContainer which will aid this process by
wrapping both the NodeID and SQLInstanceID, very similar to the work
carried out for Gossip in #47972. We do not yet audit all of the call
sites to keep the initial diff container. Instead, IDContainer has a
Get() method that makes it a drop-in replacement for the previously used
NodeIDContainer. Get will be removed in a future commit while moving
all callers to either the Optional{,Err} or DeprecatedNodeID
methods.

Release note: None

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@blathers-crl

This comment has been minimized.

@blathers-crl

This comment has been minimized.

@tbg tbg force-pushed the no-deid branch 3 times, most recently from 0290ec8 to 87cbf4b Compare April 28, 2020 16:52
@blathers-crl

This comment has been minimized.

@tbg tbg force-pushed the no-deid branch 2 times, most recently from f34f6b3 to 83ce9e9 Compare April 29, 2020 10:00
@tbg tbg marked this pull request as ready for review April 29, 2020 10:01
@tbg tbg requested a review from nvanbenschoten April 29, 2020 10:01
@tbg tbg added the A-multitenancy Related to multi-tenancy label Apr 29, 2020
@tbg tbg requested a review from asubiotto April 29, 2020 10:02
@blathers-crl
Copy link

blathers-crl bot commented Apr 29, 2020

❌ The GitHub CI (Cockroach) build has failed on 83ce9e95.

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

Copy link
Contributor

@asubiotto asubiotto left a comment

Choose a reason for hiding this comment

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

:lgtm: are you planning on adding another commit to add the Optional calls or will that be in another PR?

Reviewed 17 of 17 files at r1.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @nvanbenschoten)

@tbg
Copy link
Member Author

tbg commented Apr 29, 2020

TFTR! I'll do that here, I just wanted to get a first round of review before I make code changes that could potentially rot. Will ping again when it's ready.

Copy link
Member

@nvanbenschoten nvanbenschoten 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 17 of 17 files at r1.
Reviewable status: :shipit: complete! 2 of 0 LGTMs obtained (waiting on @tbg)


pkg/base/node_id.go, line 88 at r1 (raw file):

type SQLInstanceID int64

// IDContainer wraps a SQLInstanceID and optionally a NodeID.

Should this be called SQLIDContainer? The key-value layer should never need one of these, so it's not a direct replacement for NodeIDContainer.


pkg/base/node_id.go, line 116 at r1 (raw file):

		fmt.Fprintf(&buf, "@n%s,", n)
	}
	return ""

I don't think this is what you meant.

tbg added 3 commits May 1, 2020 13:52
With the advent of multi-tenancy, SQL servers can optionally be
decoupled from the (and share a) KV backend. As a consequence, the
NodeID may not be available and its use across all components that need
to function in a multi-tenant setup is deprecated. Yet, some components
require a similar unique identifier for the SQL server they're a part
of.

This new identifier is the SQLInstanceID. At the time of writing, since
we're not yet able to start true SQL tenant servers and only exercise
parts of this functionality in unit testing, it is a randomized integer.
It's possible that we'll replace it with a per-tenant or even global
counter in production, putting it truly on par with the NodeID, to be
able to give the simple guarantee that these IDs are never reused
between different incarnations of the SQL process for a given tenant.
This final decision is kicked down the road, see cockroachdb#48009.

This commit introduces an IDContainer which will aid this process by
wrapping both the NodeID and SQLInstanceID, very similar to the work
carried out for Gossip in cockroachdb#47972. We do not yet audit all of the call
sites to keep the initial diff container. Instead, IDContainer has a
Get() method that makes it a drop-in replacement for the previously used
NodeIDContainer. `Get ` will be removed in a future commit while moving
all callers to either the `Optional{,Err}` or `DeprecatedNodeID`
methods.

Release note: None
tbg added 7 commits May 1, 2020 14:37
This is only used to log to the (per-tenant) event log.

Release note: None
This allows its use for the cluster(= tenant's logical cluster)-wide
unique IDs generator.

Release note: None
It's used to initialize a txn, where we want to pass zero if we don't
have a NodeID.

Release note: None
@blathers-crl

This comment has been minimized.

@tbg
Copy link
Member Author

tbg commented May 1, 2020

bors r=asubiotto,nvanbenschoten

I packed on a lot of commits that remove uses of Get(). Will merge so that it doesn't rot over the weekend, but I'll send another PR with any additional review comments (also, I'll remove Get()).

@craig
Copy link
Contributor

craig bot commented May 1, 2020

Build succeeded

@craig craig bot merged commit 712a999 into cockroachdb:master May 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-multitenancy Related to multi-tenancy
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants