sql/multiregion: Add basic validation for zone config extension setting#86538
Conversation
|
This change is |
rafiss
left a comment
There was a problem hiding this comment.
just wondering, should some of this validation go into validateMultiRegion in pkg/sql/catalog/dbdesc/database_desc.go. this is validation that's lower-level at the descriptor level, so it prevents any other change to the descriptor from doing something invalid.
Reviewable status:
complete! 0 of 0 LGTMs obtained
rafiss
left a comment
There was a problem hiding this comment.
i think we might want the validation in both places. the ones in this PR should show user-friendly errors, and the ones in validateMultiRegion should show assertion errors.
Reviewable status:
ZhouXing19
left a comment
There was a problem hiding this comment.
I'm not sure if it's necessary to add the validation in the descriptor level, as the validation we have here are mostly for the zone config itself. The logic here is similar to the existing validations for the set zone config logic, which IIUC are mostly done in the sql package rather than the descriptor level (example).
Reviewable status:
41a20e0 to
3397fa2
Compare
3397fa2 to
f780d6d
Compare
|
For the requirement that |
|
this lgtm, but i could also use an opinion from @RichardJCai |
RichardJCai
left a comment
There was a problem hiding this comment.
Mostly LGTM good checks. Just some nits
pkg/sql/alter_database.go
Outdated
| // tables in it, we now follow these steps: | ||
| // 1. Generate and Validate the newly created zone config for the database. | ||
| // 2. For each table under this database, validate the newly derived zone | ||
| // config. After the validation passed for all table, we are now sure that the |
| // replicas. We may want to consider adding constraint to their distribution | ||
| // across zones/regions as well. | ||
| if zc.NumVoters != nil && *zc.NumVoters < numVoters { | ||
| return zonepb.ZoneConfig{}, errors.Newf("zone config extension "+ |
There was a problem hiding this comment.
nit: cannot set num_voters lower than ...
| } | ||
| if zc.NumReplicas != nil && *zc.NumReplicas < numReplicas { | ||
| return zonepb.ZoneConfig{}, errors.Newf("zone config extension "+ | ||
| "cannot set a replica number %v that is lower than the one required for the "+ |
There was a problem hiding this comment.
num_replicas just to be explicit about the setting name
| oriHomeRegionLeasePreference, idx1 := getHomeRegionLeasePreference(zc.LeasePreferences, homeRegion) | ||
| extHomeRegionLeasePreference, idx2 := getHomeRegionLeasePreference(ext.LeasePreferences, homeRegion) | ||
| if idx1 != -1 { | ||
| // If can' find a lease preference for the home region in the zone config |
| // field. | ||
| // Lease preferences. | ||
| if !(ext.ShouldInheritLeasePreferences(&zc)) && len(zc.LeasePreferences) > 0 && len(ext.LeasePreferences) > 0 { | ||
| oriHomeRegionLeasePreference, idx1 := getHomeRegionLeasePreference(zc.LeasePreferences, homeRegion) |
There was a problem hiding this comment.
Rather we just use originalHomeRegionLeasePreference took me a second to understand what ori meant
| if !(ext.ShouldInheritConstraints(&zc)) && len(zc.Constraints) > 0 && len(ext.Constraints) > 0 { | ||
| oriHomeRegionReplicaConstraint, ok1 := getHomeRegionConstraintConjunction(zc.Constraints, homeRegion) | ||
| extHomeRegionReplicaConstraint, ok2 := getHomeRegionConstraintConjunction(ext.Constraints, homeRegion) | ||
| if ok1 { |
There was a problem hiding this comment.
This is a little bit hard to read.
Can we do if ok1 && !ok2 { ... } and split out the logic below
Also probably better to do name ok1 as homeRegionConstraintExistsForOldZC and ok2 as homeRegionConstraintExistsForExt as something like, I prefer verbosity here to make it clear.
There was a problem hiding this comment.
I changed it to return the reference of the constraint, so the second return value is not very much needed.
f780d6d to
70d0ac7
Compare
RichardJCai
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/catalog/multiregion/region_config.go line 527 at r3 (raw file):
// If we found the constraint for the region, we return the corresponding // ConstraintsConjunction and "true" as the second boolean returned value. // If not found in the list, it returns an empty lease preference and "false".
This comment is out of date now
We added constraints on setting the zone config extension so that it won't break the original region configs (such as home region and survival goal requirement). We didn't exhaust all rules but added the following: 1. REGIONAL extensions don't set lease_preferences 2. REGIONAL IN extensions don't set lease_preferences that disagree with their home region. I.e. After the extension gets applied, the home region is unchanged. 3. REGIONAL IN extensions don't set replica constraints or voter contains that disagree with their home region. 4. Extensions can't set the number of replicas and voters lower than the ones required by the database's survival goal. Note that the validation for zone config extension is different from other zone config settings in that it will influence the derived zone config for not just the database, but also the tables and partitions under it. So we need to ensure the validation pass for all these schemas before truly write the zone config updates. fixes cockroachdb#85015 Release justification: important validation for a multiregion funtionality. Release note: add basic validation for zone config extension setting
70d0ac7 to
292736e
Compare
ZhouXing19
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/catalog/multiregion/region_config.go line 527 at r3 (raw file):
Previously, RichardJCai (Richard Cai) wrote…
This comment is out of date now
Done.
|
TFTR! |
|
Build succeeded: |
We added constraints on setting the zone config extension so that it won't break
the original region configs (such as home region and survival goal requirement).
We didn't exhaust all rules but added the following:
region. I.e. After the extension gets applied, the home region is unchanged.
with their home region.
required by the database's survival goal.
Note that the validation for zone config extension is different from other
zone config settings in that it will influence the derived zone config for not
just the database, but also the tables and partitions under it. So we need to
ensure the validation pass for all these schemas before truly write the zone
config updates.
fixes #85015
Release justification: important validation for a multiregion funtionality.
Release note: add basic validation for zone config extension setting