sql: gate global_reads zone attribute on cluster version and enterprise license #62764
Conversation
|
This change is |
| defer utilccl.TestingEnableEnterprise()() | ||
|
|
||
| _, sqlDB, cleanup := multiregionccltestutils.TestingCreateMultiRegionCluster( | ||
| t, 3 /* numServers */, base.TestingKnobs{}, nil, /* baseDir */ |
There was a problem hiding this comment.
nit: you can probably get away with just one server for this
| c.InheritedVoterConstraints = false | ||
| }, | ||
| checkAllowed: func(ctx context.Context, execCfg *ExecutorConfig, _ tree.Datum) error { | ||
| return checkVersionActive(ctx, execCfg, clusterversion.NonVotingReplicas, "voter_constraints") |
There was a problem hiding this comment.
can you have voter constraints with global_read=false? if so, do we want that enterprise only or nah?
…se license Fixes cockroachdb#61039. This commit gates the use of the global_reads zone configuration attribute on the cluster version and on the use of an enterprise license. The first half of this is necessary for correctness. The second half is not, because follower reads won't be served from global read followers without an enterprise license, but a check when the zone configs are set improves UX.
nvanbenschoten
force-pushed
the
nvanbenschoten/gateGlobalReads
branch
from
0ee4687 to
42f7f08
Compare
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @aayushshah15, @awoods187, and @otan)
pkg/ccl/multiregionccl/multiregion_test.go, line 127 at r1 (raw file):
Previously, otan (Oliver Tan) wrote…
nit: you can probably get away with just one server for this
Good point, done.
pkg/sql/set_zone_config.go, line 137 at r1 (raw file):
Previously, otan (Oliver Tan) wrote…
can you have voter constraints with global_read=false? if so, do we want that enterprise only or nah?
This is a good question. Follower reads are enterprise-only and there's not a great reason to create non-voting replicas if you can't read from them, so I don't think there's much of a reason to allow non-voting replicas without a license. But I also don't think we explicitly decided to disallow them. Or at least, non-voting replicas aren't mentioned in the "Enterprise License & Multi-region" doc.
cc. @aayushshah15 and @awoods187
There was a problem hiding this comment.
LGTM
Reviewable status:
pkg/sql/set_zone_config.go, line 137 at r1 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
This is a good question. Follower reads are enterprise-only and there's not a great reason to create non-voting replicas if you can't read from them, so I don't think there's much of a reason to allow non-voting replicas without a license. But I also don't think we explicitly decided to disallow them. Or at least, non-voting replicas aren't mentioned in the "Enterprise License & Multi-region" doc.
cc. @aayushshah15 and @awoods187
This sounds good. There isn't any reason to be able to create non-voters (won't be in 21.1, at least) without being able to follower-read, so requiring an enterprise license makes sense to me (unless Andy has objections).
|
TFTRs!
Will leave this for a follow-up PR if we decide to proceed with locking more behind an enterprise license. bors r+ |
|
Build succeeded: |
Fixes #61039.
This commit gates the use of the global_reads zone configuration
attribute on the cluster version and on the use of an enterprise
license.
The first half of this is necessary for correctness. The second half is
not, because follower reads won't be served from global read followers
without an enterprise license, but a check when the zone configs are set
improves UX.