Skip to content

Commit

Permalink
Merge #57184 #59407 #59541
Browse files Browse the repository at this point in the history 
57184: config: introduce `num_voters` and `voter_constraints` r=andreimatei,nvanbenschoten a=aayushshah15

This commit introduces two new attributes to zone configs: `num_voters`
and `voter_constraints`. These attributes semantically act as subsets of
the existing `num_replicas` and `constraints` attributes, in that they
apply _only_ to voting replicas.

After this change, when `num_voters` is explicitly specified,
`num_replicas` indicates the sum of voting and non-voting replicas.
Likewise, the existing `constraints` field governs the placement
constraints for all replicas (similarly defaulting to just voting
replicas if non-voting replicas aren't used), whereas the new
`voter_constraints` field governs placement constraints for just the
voting replicas. *This retains backwards compatibility* for existing
zone configs that do not use these newly added attributes.

Note that the set of nodes satisfying `voter_constraints` may not
necessarily be a subset of the set of nodes satisfying `constraints`.
`voter_constraints` are simply required to be compatible with
`constraints`. They can be totally disjoint from the `constraints` (i.e.
talk about completely different set of attributes).

Fixes #57505

Release note(sql change): Zone configs now support new attributes
`num_voters` and `voter_constraints`. `num_voters` will specify the
number of voting replicas. When `num_voters` is explicitly specified,
`num_replicas` will be the sum of voting and non-voting replicas.
`voter_constraints` will specify the constraints that govern the
placement of just the voting replicas, whereas the existing
`constraints` attributes will govern the placement of all replicas
(voting as well as non-voting).


59407: sql: Implement ALTER TABLE ... LOCALITY REGIONAL BY TABLE from GLOBAL r=arulajmani,otan a=ajstorm

Implements ALTER TABLE ... LOCALITY REGIONAL BY TABLE (all permutations)
from LOCALITY GLOBAL.

Release note (sql change): Implements ALTER TABLE ... LOCALITY REGIONAL
BY TABLE from LOCALITY GLOBAL.

Resolves #58343.
Also resolves #59249.

59541: storage,kv: rationalize the NewUnIndexedBatch interface and implement… r=sumeerbhola a=sumeerbhola

…ation

Release note: None

Co-authored-by: Aayush Shah <[email protected]>
Co-authored-by: Adam Storm <[email protected]>
Co-authored-by: sumeerbhola <[email protected]>
  • Loading branch information
@aayushshah15 @ajstorm @sumeerbhola
4 people committed Jan 28, 2021
4 parents b5f4391 + 836f39c + d797b96 + e0ee8c0 commit e753849
Show file tree
Hide file tree
Showing 30 changed files with 1,105 additions and 316 deletions.
80 changes: 75 additions & 5 deletions pkg/ccl/logictestccl/testdata/logic_test/alter_table_locality
View file
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,7 @@ statement error unimplemented: implementation pending
ALTER TABLE regional_by_row SET LOCALITY REGIONAL BY TABLE

statement error unimplemented: implementation pending
ALTER TABLE regional_by_row SET LOCALITY REGIONAL BY TABLE in "ap-southeast"
ALTER TABLE regional_by_row SET LOCALITY REGIONAL BY TABLE in "ap-southeast-2"

statement error unimplemented: implementation pending
ALTER TABLE regional_by_row SET LOCALITY REGIONAL BY TABLE in PRIMARY REGION
Expand Down Expand Up @@ -193,15 +193,85 @@ DATABASE alter_locality_test ALTER DATABASE alter_locality_test CONFIGURE ZONE
constraints = '{+region=ap-southeast-2: 1, +region=ca-central-1: 1, +region=us-east-1: 1}',
lease_preferences = '[[+region=ca-central-1]]'

statement error unimplemented: implementation pending
statement ok
ALTER TABLE global SET LOCALITY REGIONAL BY TABLE

statement error unimplemented: implementation pending
ALTER TABLE global SET LOCALITY REGIONAL BY TABLE in "ap-southeast"
query TT
SHOW CREATE TABLE global
----
global CREATE TABLE public.global (
i INT8 NULL,
FAMILY "primary" (i, rowid)
) LOCALITY REGIONAL BY TABLE IN PRIMARY REGION

statement error unimplemented: implementation pending
query TT
SHOW ZONE CONFIGURATION FOR TABLE global
----
DATABASE alter_locality_test ALTER DATABASE alter_locality_test CONFIGURE ZONE USING
range_min_bytes = 134217728,
range_max_bytes = 536870912,
gc.ttlseconds = 90000,
num_replicas = 3,
constraints = '{+region=ap-southeast-2: 1, +region=ca-central-1: 1, +region=us-east-1: 1}',
lease_preferences = '[[+region=ca-central-1]]'

# Alter back, to get back to original state
statement ok
ALTER TABLE global SET LOCALITY GLOBAL

statement ok
ALTER TABLE global SET LOCALITY REGIONAL BY TABLE in "ap-southeast-2"

query TT
SHOW CREATE TABLE global
----
global CREATE TABLE public.global (
i INT8 NULL,
FAMILY "primary" (i, rowid)
) LOCALITY REGIONAL BY TABLE IN "ap-southeast-2"

query TT
SHOW ZONE CONFIGURATION FOR TABLE global
----
TABLE global ALTER TABLE global CONFIGURE ZONE USING
range_min_bytes = 134217728,
range_max_bytes = 536870912,
gc.ttlseconds = 90000,
global_reads = true,
num_replicas = 3,
constraints = '{+region=ap-southeast-2: 3}',
lease_preferences = '[[+region=ap-southeast-2]]'

# Alter back, to get back to original state
statement ok
ALTER TABLE global SET LOCALITY GLOBAL

statement ok
ALTER TABLE global SET LOCALITY REGIONAL BY TABLE in PRIMARY REGION

query TT
SHOW CREATE TABLE global
----
global CREATE TABLE public.global (
i INT8 NULL,
FAMILY "primary" (i, rowid)
) LOCALITY REGIONAL BY TABLE IN PRIMARY REGION

query TT
SHOW ZONE CONFIGURATION FOR TABLE global
----
DATABASE alter_locality_test ALTER DATABASE alter_locality_test CONFIGURE ZONE USING
range_min_bytes = 134217728,
range_max_bytes = 536870912,
gc.ttlseconds = 90000,
num_replicas = 3,
constraints = '{+region=ap-southeast-2: 1, +region=ca-central-1: 1, +region=us-east-1: 1}',
lease_preferences = '[[+region=ca-central-1]]'

# Alter back, to get back to original state
statement ok
ALTER TABLE global SET LOCALITY GLOBAL

statement ok
ALTER TABLE global SET LOCALITY GLOBAL

Expand Down
2 changes: 1 addition & 1 deletion pkg/ccl/storageccl/engineccl/encrypted_fs_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -240,7 +240,7 @@ func TestPebbleEncryption(t *testing.T) {
require.Equal(t, int32(enginepbccl.EncryptionType_AES128_CTR), stats.EncryptionType)
t.Logf("EnvStats:\n%+v\n\n", *stats)

batch := db.NewUnIndexedBatch(false /* supportReader */)
batch := db.NewUnindexedBatch(true /* writeOnly */)
require.NoError(t, batch.PutUnversioned(roachpb.Key("a"), []byte("a")))
require.NoError(t, batch.Commit(true))
require.NoError(t, db.Flush())
Expand Down
149 changes: 146 additions & 3 deletions pkg/config/zonepb/zone.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,7 @@ func (c *Constraint) FromString(short string) error {
func NewZoneConfig() *ZoneConfig {
return &ZoneConfig{
InheritedConstraints: true,
InheritedVoterConstraints: true,
InheritedLeasePreferences: true,
}
}
Expand All @@ -184,10 +185,12 @@ func NewZoneConfig() *ZoneConfig {
func EmptyCompleteZoneConfig() *ZoneConfig {
return &ZoneConfig{
NumReplicas: proto.Int32(0),
NumVoters: proto.Int32(0),
RangeMinBytes: proto.Int64(0),
RangeMaxBytes: proto.Int64(0),
GC: &GCPolicy{TTLSeconds: 0},
InheritedConstraints: true,
InheritedVoterConstraints: true,
InheritedLeasePreferences: true,
}
}
Expand Down Expand Up @@ -240,26 +243,43 @@ func DefaultSystemZoneConfigRef() *ZoneConfig {
func (z *ZoneConfig) IsComplete() bool {
return ((z.NumReplicas != nil) && (z.RangeMinBytes != nil) &&
(z.RangeMaxBytes != nil) && (z.GC != nil) &&
(!z.InheritedConstraints) && (!z.InheritedLeasePreferences))
(!z.InheritedVoterConstraints) && (!z.InheritedConstraints) &&
(!z.InheritedLeasePreferences))
}

// ValidateTandemFields returns an error if the ZoneConfig to be written
// specifies a configuration that could cause problems with the introduction
// of cascading zone configs.
func (z *ZoneConfig) ValidateTandemFields() error {
var numConstrainedRepls int32
numVotersExplicit := z.NumVoters != nil && *z.NumVoters > 0
for _, constraint := range z.Constraints {
numConstrainedRepls += constraint.NumReplicas
}

if numConstrainedRepls > 0 && z.NumReplicas == nil {
return fmt.Errorf("when per-replica constraints are set, num_replicas must be set as well")
}

var numConstrainedVoters int32
for _, constraint := range z.VoterConstraints {
numConstrainedVoters += constraint.NumReplicas
}

if (numConstrainedVoters > 0 && z.NumVoters == nil) ||
(!numVotersExplicit && len(z.VoterConstraints) > 0) {
return fmt.Errorf("when voter_constraints are set, num_voters must be set as well")
}

if (z.RangeMinBytes != nil || z.RangeMaxBytes != nil) &&
(z.RangeMinBytes == nil || z.RangeMaxBytes == nil) {
return fmt.Errorf("range_min_bytes and range_max_bytes must be set together")
}
if !z.InheritedLeasePreferences && z.InheritedConstraints {
if numVotersExplicit {
if !z.InheritedLeasePreferences && z.InheritedVoterConstraints {
return fmt.Errorf("lease preferences can not be set unless the voter_constraints are explicitly set as well")
}
} else if !z.InheritedLeasePreferences && z.InheritedConstraints {
return fmt.Errorf("lease preferences can not be set unless the constraints are explicitly set as well")
}
return nil
Expand All @@ -286,7 +306,23 @@ func (z *ZoneConfig) Validate() error {
}
return fmt.Errorf("at least one replica is required")
case *z.NumReplicas == 2:
return fmt.Errorf("at least 3 replicas are required for multi-replica configurations")
if !(z.NumVoters != nil && *z.NumVoters > 0) {
return fmt.Errorf("at least 3 replicas are required for multi-replica configurations")
}
}
}

var numVotersExplicit bool
if z.NumVoters != nil {
numVotersExplicit = true
switch {
case *z.NumVoters <= 0:
return fmt.Errorf("at least one voting replica is required")
case *z.NumVoters == 2:
return fmt.Errorf("at least 3 voting replicas are required for multi-replica configurations")
}
if z.NumReplicas != nil && *z.NumVoters > *z.NumReplicas {
return fmt.Errorf("num_voters cannot be greater than num_replicas")
}
}

Expand Down Expand Up @@ -318,6 +354,19 @@ func (z *ZoneConfig) Validate() error {
}
}

for _, constraints := range z.VoterConstraints {
for _, constraint := range constraints.Constraints {
if constraint.Type == Constraint_DEPRECATED_POSITIVE {
return fmt.Errorf("voter_constraints must be of type 'required' (prefixed with a '+')")
}
// TODO(aayush): Allowing these makes validating `voter_constraints`
// against `constraints` harder. Revisit this decision if need be.
if constraint.Type == Constraint_PROHIBITED {
return fmt.Errorf("voter_constraints cannot contain prohibitive constraints")
}
}
}

// We only need to further validate constraints if per-replica constraints
// are in use. The old style of constraints that apply to all replicas don't
// require validation.
Expand All @@ -344,6 +393,48 @@ func (z *ZoneConfig) Validate() error {
}
}

// If we have per replica constraints inside voter_constraints, make sure
// that the number of replicas adds up to less than the number of voters.
//
// NB: We intentionally allow the number of replicas constrained by
// `constraints` plus the number of voters constrained by `voter_constraints`
// to exceed num_voters.
// For instance, the following would be a valid zone configuration:
// num_replicas = 3
// num_voters = 3
// constraints = {"+region=A": 1, "+region=B": 1, "+region=C": 1}
// voter_constraints = {"+ssd": 3}
// In the current state of our zone config validation logic, allowing examples
// like the one shown above also allows the user to walk themselves into
// unsatisfiable zone configurations like the following:
// num_replicas = 3
// num_voters = 3
// constraints = {"+region=A": 2, "+region=B": 1}
// voter_constraints = {"+region=C": 2, "+region=D": 1}
if numVotersExplicit {
if len(z.VoterConstraints) > 1 || (len(z.VoterConstraints) == 1 && z.VoterConstraints[0].NumReplicas != 0) {
var numConstrainedRepls int64
for _, constraints := range z.VoterConstraints {
if constraints.NumReplicas <= 0 {
return fmt.Errorf("constraints must apply to at least one replica")
}
numConstrainedRepls += int64(constraints.NumReplicas)
}
// NB: These nil checks are not required in production code but they are
// for testing as some tests run `Validate()` on incomplete zone configs.
if z.NumVoters != nil && numConstrainedRepls > int64(*z.NumVoters) {
return fmt.Errorf("the number of replicas specified in voter_constraints (%d) cannot be greater "+
"than the number of voters configured for the zone (%d)",
numConstrainedRepls, *z.NumVoters)
}
}
}

// Validate that `constraints` aren't incompatible with `voter_constraints`.
if err := validateVoterConstraintsCompatibility(z.VoterConstraints, z.Constraints); err != nil {
return err
}

for _, leasePref := range z.LeasePreferences {
if len(leasePref.Constraints) == 0 {
return fmt.Errorf("every lease preference must include at least one constraint")
Expand All @@ -359,6 +450,35 @@ func (z *ZoneConfig) Validate() error {
return nil
}

// validateVoterConstraintsCompatibility cross-validates `voter_constraints`
// against `constraints` and ensures that nothing that is prohibited at the
// overall `constraints` level is required at the `voter_constraints` level,
// since this sort of incongruity will lead to an unsatisfiable zone
// configuration.
func validateVoterConstraintsCompatibility(
voterConstraints, overallConstraints []ConstraintsConjunction,
) error {
// We know that prohibitive constraints are not allowed under
// `voter_constraints`. Walk through overallConstraints to ensure that none of
// the prohibitive constraints conflict with the `required` constraints in
// voterConstraints.
for _, constraints := range overallConstraints {
for _, constraint := range constraints.Constraints {
if constraint.Type == Constraint_PROHIBITED {
for _, otherConstraints := range voterConstraints {
for _, otherConstraint := range otherConstraints.Constraints {
conflicting := otherConstraint.Value == constraint.Value && otherConstraint.Key == constraint.Key
if conflicting {
return fmt.Errorf("prohibitive constraint %s conflicts with voter_constraint %s", constraint, otherConstraint)
}
}
}
}
}
}
return nil
}

// InheritFromParent hydrates a zones missing fields from its parent.
func (z *ZoneConfig) InheritFromParent(parent *ZoneConfig) {
// Allow for subzonePlaceholders to inherit fields from parents if needed.
Expand All @@ -367,6 +487,11 @@ func (z *ZoneConfig) InheritFromParent(parent *ZoneConfig) {
z.NumReplicas = proto.Int32(*parent.NumReplicas)
}
}
if z.NumVoters == nil || (z.NumVoters != nil && *z.NumVoters == 0) {
if parent.NumVoters != nil {
z.NumVoters = proto.Int32(*parent.NumVoters)
}
}
if z.RangeMinBytes == nil {
if parent.RangeMinBytes != nil {
z.RangeMinBytes = proto.Int64(*parent.RangeMinBytes)
Expand All @@ -389,6 +514,12 @@ func (z *ZoneConfig) InheritFromParent(parent *ZoneConfig) {
z.InheritedConstraints = false
}
}
if z.InheritedVoterConstraints {
if !parent.InheritedVoterConstraints {
z.VoterConstraints = parent.VoterConstraints
z.InheritedVoterConstraints = false
}
}
if z.InheritedLeasePreferences {
if !parent.InheritedLeasePreferences {
z.LeasePreferences = parent.LeasePreferences
Expand All @@ -406,6 +537,12 @@ func (z *ZoneConfig) CopyFromZone(other ZoneConfig, fieldList []tree.Name) {
z.NumReplicas = proto.Int32(*other.NumReplicas)
}
}
if fieldName == "num_voters" {
z.NumVoters = nil
if other.NumVoters != nil {
z.NumVoters = proto.Int32(*other.NumVoters)
}
}
if fieldName == "range_min_bytes" {
z.RangeMinBytes = nil
if other.RangeMinBytes != nil {
Expand Down Expand Up @@ -435,6 +572,10 @@ func (z *ZoneConfig) CopyFromZone(other ZoneConfig, fieldList []tree.Name) {
z.Constraints = other.Constraints
z.InheritedConstraints = other.InheritedConstraints
}
if fieldName == "voter_constraints" {
z.VoterConstraints = other.VoterConstraints
z.InheritedVoterConstraints = other.InheritedVoterConstraints
}
if fieldName == "lease_preferences" {
z.LeasePreferences = other.LeasePreferences
z.InheritedLeasePreferences = other.InheritedLeasePreferences
Expand Down Expand Up @@ -500,6 +641,8 @@ func (z *ZoneConfig) IsSubzonePlaceholder() bool {
// A ZoneConfig with zero replicas is otherwise invalid, so we repurpose it to
// indicate that a ZoneConfig is a placeholder for subzones rather than
// introducing a dedicated IsPlaceholder flag.
// TODO(aayush): Decide whether its worth introducing a isPlaceholder flag to
// clean this up after num_voters is introduced.
return z.NumReplicas != nil && *z.NumReplicas == 0
}

Expand Down
Loading

0 comments on commit e753849

Please sign in to comment.