sql: mark use of Gossip as deprecated #47972
Conversation
|
This change is |
This comment has been minimized.
This comment has been minimized.
|
❌ The GitHub CI (Cockroach) build has failed on d0896b5c. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan. |
There was a problem hiding this comment.
Reviewed 31 of 31 files at r1.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @nvanbenschoten and @tbg)
pkg/gossip/gossip.go, line 1621 at r1 (raw file):
// MakeDeprecatedGossip initializes a DeprecatedGossip instance. See // MakeTenantSQLDeprecatedWrapper for details. func MakeDeprecatedGossip(g *Gossip, tenant bool) DeprecatedGossip {
What can we do about disallowing new uses of Gossip?
pkg/util/errorutil/tenant_deprecated_wrapper.go, line 31 at r1 (raw file):
// be available in a SQL tenant server for the time being. Such calls may also // point at work items that may obviate the use of the wrapper in the future. func MakeTenantSQLDeprecatedWrapper(v interface{}, tenant bool) TenantSQLDeprecatedWrapper {
After reading this comment I'm still not 100% sure what the tenant boolean is specifying. I guess I'm confused by the "indicates whether the object is part of a SQL tenant server". What exactly does that mean? Based on the behavior below I guess it's whether the wrapped object is unavailable in a SQL tenant server? I think it might be clearer if you make the behavior more explicit e.g. s/tenant/forceUnavailability.
There was a problem hiding this comment.
Reviewed 31 of 31 files at r1.
Reviewable status:
pkg/gossip/gossip.go, line 1631 at r1 (raw file):
// // See the comments on TenantSQLDeprecatedWrapper for details. type DeprecatedGossip struct {
nit: is type DeprecatedGossip errorutil.TenantSQLDeprecatedWrapper a cleaner pattern to start with these option types?
pkg/sql/distsql/server.go, line 51 at r1 (raw file):
const minFlowDrainWait = 1 * time.Second const distSQLGossipIssueNo = 47900
nit: consider commenting with the URL so that it's easier to jump to.
pkg/util/errorutil/tenant.go, line 33 at r1 (raw file):
} } return errors.New("operation is unsupported in multi-tenancy mode" + buf.String())
nit: might as well avoid the string concatenation by pulling up a buf.WriteString("operation is unsupported in multi-tenancy mode"). That will also make this easier to read.
pkg/util/errorutil/tenant_deprecated_wrapper.go, line 31 at r1 (raw file):
Previously, asubiotto (Alfonso Subiotto Marqués) wrote…
After reading this comment I'm still not 100% sure what the
tenantboolean is specifying. I guess I'm confused by the "indicates whether the object is part of a SQL tenant server". What exactly does that mean? Based on the behavior below I guess it's whether the wrapped object is unavailable in a SQL tenant server? I think it might be clearer if you make the behavior more explicit e.g.s/tenant/forceUnavailability.
I agree. After reading this file, I'm still not sure I understand the difference between tenant and v == nil. Is there one? Is the idea that we'll still keep providing dependencies even when in a SQL tenant server, but we'll slowly get rid of calls to Deprecated which use them and only then will the difference be immaterial?
I'd also consider inverting the condition so that the zero value of this struct can be used to indicate that the optional value is not present.
pkg/util/errorutil/tenant_deprecated_wrapper.go, line 50 at r1 (raw file):
// A list of related issues can optionally be provided, in case there is a // desire to make the functionality available to tenants in the future. func (w TenantSQLDeprecatedWrapper) Optional(issueNos ...int) (interface{}, error) {
Do we want a second form of this that return a boolean instead of an error when the dependency is not provided? I'd hate for us to feel like we can't add conditional logic in hot paths due to the cost of calling this method.
|
The value is always provided for the system tenant (tenant=false) to enable
the optional features like crdb_internal.
In other tenants, it has to be provided for now because there are still
calls to Deprecated() left. When those are gone, we can use a nil Gossip
here. (And can rename DeprecatedGossip to Optional gossip, hiding the
Deprecated method, and remove the bool as well (replace with g==nil).
Does that make sense? Open to suggestions for making that clearer. I can
improve verbiage in the code tomorrow but if there's a different approach
I'm happy to consider it too.
…On Thu, Apr 23, 2020, 22:53 Nathan VanBenschoten ***@***.***> wrote:
***@***.**** commented on this pull request.
Reviewed 31 of 31 files at r1.
*Reviewable <https://reviewable.io/reviews/cockroachdb/cockroach/47972>*
status: [image:
] complete! 0 of 0 LGTMs obtained (waiting on
@asubiotto <https://github.com/asubiotto> and @tbg
<https://github.com/tbg>)
------------------------------
*pkg/gossip/gossip.go, line 1631 at r1
<https://reviewable.io/reviews/cockroachdb/cockroach/47972#-M5cnZb56E5uSKJLO1r9:-M5cnZb56E5uSKJLO1rA:by56pk8>
(raw file
<https://github.com/cockroachdb/cockroach/blob/d0896b5cb7bfc2880c963c8dc6b3e24c536ce039/pkg/gossip/gossip.go#L1631>):*
//
// See the comments on TenantSQLDeprecatedWrapper for details.
type DeprecatedGossip struct {
nit: is type DeprecatedGossip errorutil.TenantSQLDeprecatedWrapper a
cleaner pattern to start with these option types?
------------------------------
*pkg/sql/distsql/server.go, line 51 at r1
<https://reviewable.io/reviews/cockroachdb/cockroach/47972#-M5cnwycEApwi1SNhvPL:-M5cnwyd7EZ0pIdyAu5o:bsf9ozz>
(raw file
<https://github.com/cockroachdb/cockroach/blob/d0896b5cb7bfc2880c963c8dc6b3e24c536ce039/pkg/sql/distsql/server.go#L51>):*
const minFlowDrainWait = 1 * time.Second
const distSQLGossipIssueNo = 47900
nit: consider commenting with the URL so that it's easier to jump to.
------------------------------
*pkg/util/errorutil/tenant.go, line 33 at r1
<https://reviewable.io/reviews/cockroachdb/cockroach/47972#-M5ckeWx6EuCMx2Z9IrV:-M5ckeWx6EuCMx2Z9IrW:bl10dac>
(raw file
<https://github.com/cockroachdb/cockroach/blob/d0896b5cb7bfc2880c963c8dc6b3e24c536ce039/pkg/util/errorutil/tenant.go#L33>):*
}
}
return errors.New("operation is unsupported in multi-tenancy mode" + buf.String())
nit: might as well avoid the string concatenation by pulling up a buf.WriteString("operation
is unsupported in multi-tenancy mode"). That will also make this easier
to read.
------------------------------
*pkg/util/errorutil/tenant_deprecated_wrapper.go, line 31 at r1
<https://reviewable.io/reviews/cockroachdb/cockroach/47972#-M5btXhq2dHyYGryrw0n:-M5clK0b0eYh5F3MXowW:b-fgscta>
(raw file
<https://github.com/cockroachdb/cockroach/blob/d0896b5cb7bfc2880c963c8dc6b3e24c536ce039/pkg/util/errorutil/tenant_deprecated_wrapper.go#L31>):*
*Previously, asubiotto (Alfonso Subiotto Marqués) wrote…*
After reading this comment I'm still not 100% sure what the tenant
boolean is specifying. I guess I'm confused by the "indicates whether the
object is part of a SQL tenant server". What exactly does that mean? Based
on the behavior below I guess it's whether the wrapped object is
unavailable in a SQL tenant server? I think it might be clearer if you make
the behavior more explicit e.g. s/tenant/forceUnavailability.
I agree. After reading this file, I'm still not sure I understand the
difference between tenant and v == nil. Is there one? Is the idea that
we'll still keep providing dependencies even when in a SQL tenant server,
but we'll slowly get rid of calls to Deprecated which use them and only
then will the difference be immaterial?
I'd also consider inverting the condition so that the zero value of this
struct can be used to indicate that the optional value is not present.
------------------------------
*pkg/util/errorutil/tenant_deprecated_wrapper.go, line 50 at r1
<https://reviewable.io/reviews/cockroachdb/cockroach/47972#-M5cleha2bxke6HE9nzQ:-M5clehb147ITnqBR1Il:b-dj6fkd>
(raw file
<https://github.com/cockroachdb/cockroach/blob/d0896b5cb7bfc2880c963c8dc6b3e24c536ce039/pkg/util/errorutil/tenant_deprecated_wrapper.go#L50>):*
// A list of related issues can optionally be provided, in case there is a
// desire to make the functionality available to tenants in the future.
func (w TenantSQLDeprecatedWrapper) Optional(issueNos ...int) (interface{}, error) {
Do we want a second form of this that return a boolean instead of an error
when the dependency is not provided? I'd hate for us to feel like we can't
add conditional logic in hot paths due to the cost of calling this method.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#47972 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABGXPZFDUPBAILVJQK2PFH3ROCTDNANCNFSM4MPAOKDQ>
.
|
There was a problem hiding this comment.
Reviewable status:
pkg/gossip/gossip.go, line 1621 at r1 (raw file):
Previously, asubiotto (Alfonso Subiotto Marqués) wrote…
What can we do about disallowing new uses of
Gossip?
I don't think we need to do more. If you wanted to use Gossip, you would have to get it from the DeprecatedGossip, and it will pretty much force you to realize that you are setting yourself up for failure unless the new use is optional (and even then, it cautions you away from it). Did you have something else in mind or do you think this isn't enough?
pkg/gossip/gossip.go, line 1631 at r1 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
nit: is
type DeprecatedGossip errorutil.TenantSQLDeprecatedWrappera cleaner pattern to start with these option types?
When I first read this suggestion I thought so, but I'm less sure now. It allows you to cast back to the wrapper type, which has the Deprecated method which I want to remove from DeprecatedGossip as soon as I can. Theoretical concern, but it's a concern and I don't see anything in the other choice that would outweigh it.
pkg/util/errorutil/tenant_deprecated_wrapper.go, line 31 at r1 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
I agree. After reading this file, I'm still not sure I understand the difference between
tenantandv == nil. Is there one? Is the idea that we'll still keep providing dependencies even when in a SQL tenant server, but we'll slowly get rid of calls toDeprecatedwhich use them and only then will the difference be immaterial?I'd also consider inverting the condition so that the zero value of this struct can be used to indicate that the optional value is not present.
I re-worked the commentary to (hopefully) clear this confusion up for all future readers. WDYT?
pkg/util/errorutil/tenant_deprecated_wrapper.go, line 50 at r1 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
Do we want a second form of this that return a boolean instead of an error when the dependency is not provided? I'd hate for us to feel like we can't add conditional logic in hot paths due to the cost of calling this method.
I added it but I don't really like it. These are harder to track, and they allow inconsistent errors of the form
if g, ok := w.Optional(); !ok {
return errors.New("thing I wanted is missing")
}
That said, you're probably right that we'll see something pop up in the hot path that actually needs to stick around. I'll leave it in and we can revisit. When we give the NodeID a similar treatment (#48009) we'll probably need it.
tbg
force-pushed
the
deprecate-gossip
branch
2 times, most recently
from
01e51b9
to
ed47ce0
Compare
This commit wraps Gossip in a wrapper when it is used in SQL. This small wrapper helpfully keeps track, in code, of work that needs to be done to enable multi-tenancy, and ensures that any deprecated use of Gossip is associated with a Github issue. The wrapper also makes sure we're not introducing new dependencies on Gossip. Note that this wrapper is more than an option type: it wraps an object that is always there (at least at the beginning), but we can configure the wrapper to pretend that it isn't there (for callers that can handle that). Release note: None
There was a problem hiding this comment.
Reviewed 6 of 13 files at r2.
Reviewable status:
pkg/gossip/gossip.go, line 1621 at r1 (raw file):
Previously, tbg (Tobias Grieger) wrote…
I don't think we need to do more. If you wanted to use Gossip, you would have to get it from the DeprecatedGossip, and it will pretty much force you to realize that you are setting yourself up for failure unless the new use is optional (and even then, it cautions you away from it). Did you have something else in mind or do you think this isn't enough?
SGTM
There was a problem hiding this comment.
Reviewed 10 of 13 files at r2.
Reviewable status:
|
TFTRs! bors r=asubiotto,nvanbenschoten |
Build succeeded |
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
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
48110: base: introduce SQLInstanceID (and container) r=asubiotto,nvanbenschoten a=tbg 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 Co-authored-by: Tobias Schottdorf <tobias.schottdorf@gmail.com>
This commit wraps Gossip in a wrapper when it is used in SQL. This small
wrapper helpfully keeps track, in code, of work that needs to be done to
enable multi-tenancy, and ensures that any deprecated use of Gossip is
associated with a Github issue. The wrapper also makes sure we're not
introducing new dependencies on Gossip.
Release note: None