sql, config: make zone configs cascade #30611

ridwanmsharif · Sep 25, 2018

Modifies the ZoneConfig struct to
make fields optional now. If fields are not set, they
are looked for in the heirarchy. Refer to the RFC for
more details.

Issue: #28901
Implements most of RFC: #30426

TODO:

Update set_zone_config to use the new structure and validate them
Exhaustively test cascading zone configs

Release note: Changes behavior of Zone Configs.

This change is

cockroach-teamcity · Sep 25, 2018

This change is

benesch · Oct 1, 2018

benesch reviewed Oct 1, 2018

View changes

This is coming along really nicely! I left you a bunch of comments below.

One rule of thumb is to try to keep PRs to under 500 lines of code. More than that and your reviewers get overwhelmed—plus it's much harder to keep up-to-date, since every rebase will have a lot of conflicts.

This nature of this change is unfortunately going to be very hard to split in two. How hard would it be to extract the proto nullability change into its own PR? That'll remove most of the weight from this PR, and I think we could get that half merged today or early tomorrow.

Reviewed 50 of 50 files at r1, 15 of 15 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained

pkg/cli/demo.go, line 103 at r1 (raw file):

	cfg := config.DefaultZoneConfig()
	cfg.NumReplicas = proto.Int32(1)
	restoreCfg := config.TestingSetDefaultSystemZoneConfig(cfg)

Hm, the existing code here seems wrong. Can you add a TODO for me?

TODO(benesch): should this use TestingSetDefaultZoneConfig instead fo TestingSetDefaultSystemZoneConfig?

pkg/config/system.go, line 43 at r1 (raw file):

	ZoneConfigHook zoneConfigHook

	// InheritFromParent is a function that fills in the zones missing fields

nit: "zone's"

pkg/config/system.go, line 409 at r1 (raw file):

func (s *SystemConfig) NeedsSplit(startKey, endKey roachpb.RKey) bool {
	result := len(s.ComputeSplitKey(startKey, endKey)) > 0
	return result

This looks like fallout from our debugging session.

pkg/config/zone.go, line 305 at r1 (raw file):

		RangeMaxBytes: proto.Int64(*defaultZoneConfig.RangeMaxBytes),
		GC:            &GCPolicy{TTLSeconds: defaultZoneConfig.GC.TTLSeconds},
	}

I don't think it's worth going to the trouble here to deep copy the proto. We should just require that no one ever modify a field through the pointer.

(Also, for reference, better to use protoutil.Clone to avoid manually copying every field.)

pkg/config/zone.go, line 325 at r1 (raw file):

		RangeMaxBytes: proto.Int64(*defaultSystemZoneConfig.RangeMaxBytes),
		GC:            &GCPolicy{TTLSeconds: defaultSystemZoneConfig.GC.TTLSeconds},
	}

Same comment as above.

pkg/config/zone.go, line 347 at r1 (raw file):

func TestingSetDefaultSystemZoneConfig(cfg ZoneConfig) func() {
	testingLock.Lock()
	oldConfig := defaultSystemZoneConfig

😅

pkg/config/zone.go, line 386 at r1 (raw file):

	case z.NumReplicas != nil && *z.NumReplicas == 2:
		return fmt.Errorf("at least 3 replicas are required for multi-replica configurations")
	}

Maybe just wrap this whole switch in a if z.NumReplicas != nil?

pkg/config/zone.go, line 541 at r2 (raw file):

			((s.EndKey == nil && bytes.HasPrefix(keySuffix, s.Key)) || s.EndKey.Compare(keySuffix) > 0) {
			copySubzone := z.Subzones[s.SubzoneIndex]
			return &copySubzone

Hmm, is this strictly necessary?

pkg/config/zone.proto, line 135 at r1 (raw file):

  // ExplicitlySetContraints specifies if value in the Constraints field was
  // pupulated by the user explicitly and not inherited from the zone's parent.
  optional bool explicitly_set_constraints = 10 [(gogoproto.nullable) = false];

nit: prefer to keep these in semantic order, rather than numeric order. That is, stick this field after the constraints field and stick the explicitly_set_lease_preferences field after the lease_preferences field.

pkg/config/zone.proto, line 135 at r1 (raw file):

  // ExplicitlySetContraints specifies if value in the Constraints field was
  // pupulated by the user explicitly and not inherited from the zone's parent.
  optional bool explicitly_set_constraints = 10 [(gogoproto.nullable) = false];

I think these fields need to be inverted so that the default has the opposite meaning. E.g., for zone configs that were created before this PR, we want explicitly_set_constraints to be true, but they'll end up being false. I suggest inherit_constraints and inherit_lease_preferences.

pkg/config/zone_test.go, line 40 at r1 (raw file):

			ZoneConfig{},
			"at least one replica is required",
		},

I think this should test case should remain as the case where NumReplicas is set to proto.Int32(0).

pkg/config/zone_test.go, line 722 at r1 (raw file):

	// TODO(ridwanmsharif): Check if this makes sense
	defZone := DefaultZoneConfig()
	original := &defZone

Does NewPopulatedZoneConfig no longer work?

pkg/sql/set_zone_config.go, line 294 at r1 (raw file):

			didDelete := completeZone.DeleteSubzone(uint32(index.ID), partition)
			partialDidDelete := partialZone.DeleteSubzone(uint32(index.ID), partition)
			if !didDelete && !partialDidDelete {

If didDelete != partialDidDelete, we have a problem. That should be an error. Right?

pkg/sql/set_zone_config.go, line 327 at r1 (raw file):

		if partialSubzone != nil {
			finalZone = partialSubzone.Config
		}

I'm confused about why this case isn't exactly analogous to the complete case. Why do you need the extra check for index != nil?

pkg/sql/set_zone_config.go, line 335 at r1 (raw file):

		} else if n.setDefault {
			finalZone = config.ZoneConfig{}
		}

Huh. How was this working before?

pkg/sql/set_zone_config.go, line 411 at r1 (raw file):

			// The partial zone might just be empty. If so,
			// replace it with a SubzonePlaceolder.

"SubzonePlaceholder"

pkg/sql/set_zone_config.go, line 429 at r1 (raw file):

	}

	// Write the parital zone configutation.

"partial"

pkg/sql/set_zone_config.go, line 620 at r1 (raw file):

	}
	return &zone, nil
}

Rather than duplicating this method, I'd change it to return nil if no zone config exists, and then adjust the caller that wants the empty zone config to handle the nil -> empty zone config conversion.

pkg/sql/show_trace_replica_test.go, line 106 at r1 (raw file):

			// Read from table that inherits constraints from database
			query:    `SELECT * FROM d.t4`,
			expected: [][]string{{`1`, `1`}},

I'd recommend removing this test case. Since this test is about SHOW REPLICA TRACE, not zone configs, it doesn't need to test complicated details like zone config inheritance. That's better left to dedicated zone config tests.

pkg/sql/zone_config_test.go, line 245 at r1 (raw file):

	db1Cfg.ExplicitlySetConstraints = true

	tb11Cfg := db1Cfg

Why is this one based on db1Cfg when the rest are based on defaultZoneConfig.

pkg/sql/zone_config_test.go, line 331 at r2 (raw file):

// TestCascadingZoneConfig tests whether the cascading nature of
// the zone configurations works well with the different inheritance
// heirarchies.

"hierarchies"

pkg/sql/zone_config_test.go, line 654 at r2 (raw file):

		{tb22, []byte{255}, "", defaultZoneConfig},
	})
}

Hmm, this is pretty duplicative of TestGetZoneConfig. How hard would it be to fold this test into that one?

pkg/sql/zone_test.go, line 241 at r1 (raw file):

		t.Errorf("expected SHOW ZONE CONFIGURATION to fail on dropped table, but got %q", err)
	}
	sqlutils.VerifyAllZoneConfigs(t, sqlDB, defaultOverrideRow, partialSystemRow, partialJobsRow)

I forgot what our plan was here. Was it to make crdb_internal.zones also fill in the full zone config?

pkg/storage/allocator_scorer.go, line 940 at r1 (raw file):

		}
	}
	if constrainedReplicas > 0 && zone.NumReplicas != nil && constrainedReplicas < *zone.NumReplicas {

This shouldn't be necessary. Let's fix the test that's causing this?

pkg/storage/queue_test.go, line 609 at r1 (raw file):

	// The range willSplit starts at the beginning of the user data range,
	// which means keys.MaxReservedDescID+1.
	config.TestingSetZoneConfig(keys.MaxReservedDescID+2, config.ZoneConfig{RangeMaxBytes: proto.Int64(1 << 20)})

For consistency, this zone config should probably be fully fleshed out (as you've done with tests below) by starting with config.DefaultZoneConfig().

pkg/storage/replica_test.go, line 9178 at r1 (raw file):

		t.Run("", func(t *testing.T) {
			zoneConfig := config.ZoneConfig{NumReplicas: proto.Int32(0)}
			zoneConfig.NumReplicas = proto.Int32(c.replicas)

This is oddly roundabout. Did you mean to make the first line the following?

zoneConfig := config.DefaultZoneConfig()

ridwanmsharif · Oct 1, 2018

ridwanmsharif reviewed Oct 1, 2018

View changes

I was hoping to do just that but that divide didn't work very well when I tried it. The partial zone configs cause a lot of problems because now the GetZoneConfig functions no longer guaranteed completeness. So A lot more tests would need to be changed to only be changed back again later. And to guarantee completeness, the inheritance logic needed to be fleshed out. Was really unfortunate really that it had to be this way. Also, now I realise why you guys were talking about this being a "hairy" change. Thought it was very cut and dry when I took it on.

Reviewable status: complete! 0 of 0 LGTMs obtained

pkg/cli/demo.go, line 103 at r1 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

Hm, the existing code here seems wrong. Can you add a TODO for me?
TODO(benesch): should this use TestingSetDefaultZoneConfig instead fo TestingSetDefaultSystemZoneConfig?

Done.

pkg/config/system.go, line 409 at r1 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

This looks like fallout from our debugging session.

Done.

pkg/config/zone.go, line 305 at r1 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

I don't think it's worth going to the trouble here to deep copy the proto. We should just require that no one ever modify a field through the pointer.

(Also, for reference, better to use protoutil.Clone to avoid manually copying every field.)

TIL protoutil.Clone. Using that now, prefer doing that than requiring no one change the field of the default through the pointer.

pkg/config/zone.go, line 325 at r1 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

Same comment as above.

Done.

pkg/config/zone.go, line 347 at r1 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

😅

This drove me insane. Thanks a ton again for finding it @benesch.

pkg/config/zone.go, line 386 at r1 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

Maybe just wrap this whole switch in a if z.NumReplicas != nil?

Done.

pkg/config/zone.go, line 541 at r2 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

Hmm, is this strictly necessary?

I believe it is. Otherwise we have that the pointer is used to update a subzone in InheritFromParent to change the subzone config after the fact. It led to conditions where the zone returned by GetZoneConfigForKey had its subzone configs be affected after the call had been made. This prevents all such cases.

pkg/config/zone.proto, line 135 at r1 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

I think these fields need to be inverted so that the default has the opposite meaning. E.g., for zone configs that were created before this PR, we want explicitly_set_constraints to be true, but they'll end up being false. I suggest inherit_constraints and inherit_lease_preferences.

You're 100% right. I was reluctant to do that initially because proto3 doesn't let you use default values. So if a user doesn't specify a field, I didn't want inherited_filed to be false. But now I realise that we're using proto2 syntax for ZoneConfigs. So it should not be a problem really.

pkg/config/zone_test.go, line 40 at r1 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

I think this should test case should remain as the case where NumReplicas is set to proto.Int32(0).

Done.

pkg/config/zone_test.go, line 722 at r1 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

Does NewPopulatedZoneConfig no longer work?

Nope. What did this test do exactly before? I wan't completely sure so I don't know if I recreated the test accurately.

pkg/sql/set_zone_config.go, line 294 at r1 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

If didDelete != partialDidDelete, we have a problem. That should be an error. Right?

Don't think partialDidDelete is even necessary here.

pkg/sql/set_zone_config.go, line 327 at r1 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

I'm confused about why this case isn't exactly analogous to the complete case. Why do you need the extra check for index != nil?

If index != nil then the zone we write shouldn't take on the values from partialZone. It should be empty, or should take on the values of the subzone if it exists. If the index != nil condition wasn't there, it'd take on values of the partialZone which we do not want at all.

(In testing a situation came up where a subzone had subzones as a result of this check not being present)

pkg/sql/set_zone_config.go, line 335 at r1 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

Huh. How was this working before?

Wasn't a problem before because the newZone would be set to the default without this check because of GetZoneConfigInTxn. Now however we explicitly deal with the cases where the default is being reverted, and another zone is modified using USING DEFAULT (where that just means they inherit all values).

pkg/sql/set_zone_config.go, line 411 at r1 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

"SubzonePlaceholder"

Done.

pkg/sql/set_zone_config.go, line 429 at r1 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

"partial"

Done.

pkg/sql/set_zone_config.go, line 620 at r1 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

Rather than duplicating this method, I'd change it to return nil if no zone config exists, and then adjust the caller that wants the empty zone config to handle the nil -> empty zone config conversion.

Done.

pkg/sql/show_trace_replica_test.go, line 106 at r1 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

I'd recommend removing this test case. Since this test is about SHOW REPLICA TRACE, not zone configs, it doesn't need to test complicated details like zone config inheritance. That's better left to dedicated zone config tests.

Done.

pkg/sql/zone_config_test.go, line 331 at r2 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

"hierarchies"

Done.

pkg/sql/zone_config_test.go, line 654 at r2 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

Hmm, this is pretty duplicative of TestGetZoneConfig. How hard would it be to fold this test into that one?

I'd keep both because this one sets the zones partially where as that test sets complete zone configs and tests whether they are retrieved properly. This one sets partial ones and checks for completeness as well as inheritance.

pkg/sql/zone_test.go, line 241 at r1 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

I forgot what our plan was here. Was it to make crdb_internal.zones also fill in the full zone config?

For now because the output of SHOW ALL ZONE CONFIGURATIONS is different, we had these work with the partial output. After we change it to remove the yaml output and add our custom output, this will need to be changed.

pkg/storage/allocator_scorer.go, line 940 at r1 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

This shouldn't be necessary. Let's fix the test that's causing this?

Done.

pkg/storage/queue_test.go, line 609 at r1 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

For consistency, this zone config should probably be fully fleshed out (as you've done with tests below) by starting with config.DefaultZoneConfig().

Done.

pkg/storage/replica_test.go, line 9178 at r1 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

This is oddly roundabout. Did you mean to make the first line the following?
zoneConfig := config.DefaultZoneConfig()

Done.

ridwanmsharif · Oct 1, 2018

ridwanmsharif reviewed Oct 1, 2018

View changes

Reviewable status: complete! 0 of 0 LGTMs obtained

pkg/config/zone.proto, line 135 at r1 (raw file):

Previously, ridwanmsharif (Ridwan Sharif) wrote…

You're 100% right. I was reluctant to do that initially because proto3 doesn't let you use default values. So if a user doesn't specify a field, I didn't want inherited_filed to be false. But now I realise that we're using proto2 syntax for ZoneConfigs. So it should not be a problem really.

Wait no that doesn't work, then the zone configs created before this PR would also default to being inherited by default when we don't want that at all. hmmm if I'm not entirely wrong, we can check if a field hasn't been specified in the yaml unmarshaller and set the value there. I'll look into that

benesch · Oct 2, 2018

benesch reviewed Oct 2, 2018

View changes

I was hoping to do just that but that divide didn't work very well when I tried it. The partial zone configs cause a lot of problems because now the GetZoneConfig functions no longer guaranteed completeness. So A lot more tests would need to be changed to only be changed back again later. And to guarantee completeness, the inheritance logic needed to be fleshed out. Was really unfortunate really that it had to be this way.

Yeah, I was thinking you'd just do the nullability change but not the completeness change. Does that make sense? Regardless this PR is in good shape so fine to leave as is.

Also, now I realise why you guys were talking about this being a "hairy" change. Thought it was very cut and dry when I took it on.

Heh, nothing is ever as easy as it seems.

Reviewed 14 of 14 files at r3.
Reviewable status: complete! 0 of 0 LGTMs obtained

pkg/config/zone.go, line 305 at r1 (raw file):

Previously, ridwanmsharif (Ridwan Sharif) wrote…

TIL protoutil.Clone. Using that now, prefer doing that than requiring no one change the field of the default through the pointer.

Ack, but then it seems odd that you're only making a shallow copy of the subzone in

pkg/config/zone.proto, line 135 at r1 (raw file):

Previously, ridwanmsharif (Ridwan Sharif) wrote…

Wait no that doesn't work, then the zone configs created before this PR would also default to being inherited by default when we don't want that at all. hmmm if I'm not entirely wrong, we can check if a field hasn't been specified in the yaml unmarshaller and set the value there. I'll look into that

I don't follow. Zone configs created before this PR will default whatever the new field is to false. We want to default inherited to false because those zone configs will be fully hydrated. If the field is instead explicitly_set, we'll incorrectly default that to false when it should be true.

pkg/config/zone_test.go, line 722 at r1 (raw file):

Previously, ridwanmsharif (Ridwan Sharif) wrote…

Nope. What did this test do exactly before? I wan't completely sure so I don't know if I recreated the test accurately.

The point is to make sure that a fully filled out zone config can roundtrip. The default zone config uses zero values in some places; NewPopulatedZoneConfig is better at getting a fully filled out zone config. You're probably just missing a populated protobuf option somewhere.

pkg/sql/zone_config_test.go, line 654 at r2 (raw file):

Previously, ridwanmsharif (Ridwan Sharif) wrote…

I'd keep both because this one sets the zones partially where as that test sets complete zone configs and tests whether they are retrieved properly. This one sets partial ones and checks for completeness as well as inheritance.

Ack, but both of these tests could really use an update. Can you file a TODO? I can give you specifics at some later point. But the way these just hack in zone configs instead of going through the ALTER ... CONFIGURE ZONE statement is rather unfortunate.

TODO(benesch,ridwansharif): modernize these tests to avoid hardcoding expectations about descriptor IDs and zone config encoding.

pkg/storage/allocator_test.go, line 5628 at r3 (raw file):

			target, details := alloc.RebalanceTarget(
				context.Background(),
				&config.ZoneConfig{NumReplicas: proto.Int32(0)},

Oh, I see. Not a big deal, but might be cleaner to use config.DefaultZoneConfig(). Totally optional.

ridwanmsharif · Oct 2, 2018

ridwanmsharif reviewed Oct 2, 2018

View changes

Reviewable status: complete! 0 of 0 LGTMs obtained

pkg/config/zone.proto, line 135 at r1 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

I don't follow. Zone configs created before this PR will default whatever the new field is to false. We want to default inherited to false because those zone configs will be fully hydrated. If the field is instead explicitly_set, we'll incorrectly default that to false when it should be true.

Done.

pkg/config/zone_test.go, line 722 at r1 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

The point is to make sure that a fully filled out zone config can roundtrip. The default zone config uses zero values in some places; NewPopulatedZoneConfig is better at getting a fully filled out zone config. You're probably just missing a populated protobuf option somewhere.

Done.

pkg/sql/zone_config_test.go, line 654 at r2 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

Ack, but both of these tests could really use an update. Can you file a TODO? I can give you specifics at some later point. But the way these just hack in zone configs instead of going through the ALTER ... CONFIGURE ZONE statement is rather unfortunate.
TODO(benesch,ridwansharif): modernize these tests to avoid hardcoding expectations about descriptor IDs and zone config encoding.

Done.

benesch · Oct 3, 2018

benesch reviewed Oct 3, 2018

View changes

Reviewed 12 of 32 files at r4, 4 of 5 files at r5, 1 of 15 files at r6.
Reviewable status: complete! 0 of 0 LGTMs obtained

pkg/config/zone.go, line 541 at r2 (raw file):

Previously, ridwanmsharif (Ridwan Sharif) wrote…

I believe it is. Otherwise we have that the pointer is used to update a subzone in InheritFromParent to change the subzone config after the fact. It led to conditions where the zone returned by GetZoneConfigForKey had its subzone configs be affected after the call had been made. This prevents all such cases.

Ok, but what about callers that use GetSubzone instead of GetSubzoneForKeySuffix? Shouldn't that copy too?

pkg/config/zone_yaml.go, line 74 at r6 (raw file):

type ConstraintsList struct {
	Constraints   []Constraints
	ExplicitlySet bool

Is there something that makes it difficult to flip this around to Inherited so it matches the zone configs?

pkg/sql/set_zone_config.go, line 327 at r1 (raw file):

Previously, ridwanmsharif (Ridwan Sharif) wrote…

If index != nil then the zone we write shouldn't take on the values from partialZone. It should be empty, or should take on the values of the subzone if it exists. If the index != nil condition wasn't there, it'd take on values of the partialZone which we do not want at all.

(In testing a situation came up where a subzone had subzones as a result of this check not being present)

Oh, I see. I think this belongs up where partialSubzone is declared then. Can you do something like this?

var partialSubzone *config.Subzone
if index != nil {
	partialSubzone = partialZone.GetSubzone(uint32(index.ID), partition)
	if partialSubzone == nil {
		partialSubzone = &config.Subzone{Config: &config.ZoneConfig{}}
	}
}

pkg/sql/set_zone_config.go, line 273 at r3 (raw file):

	// parameter getInheritedDefault to GetZoneConfigInTxn().
	// These zones are only used for validations. The merged zone is not
	// will be written.

"is not will be"

pkg/sql/set_zone_config.go, line 344 at r3 (raw file):

		}

		// Lead settings from YAML into the partial zone as well.

"Load"

ridwanmsharif · Oct 4, 2018

ridwanmsharif reviewed Oct 4, 2018

View changes

This PR is getting a little large. I'll follow this PR up with others to change the output to 'SHOW ZONE CONFIGURATIONS' to show the origin of each field, and then complete the RFC implementation with the unsetters and the validation guarantee (Write per replica constraints and replication factor together. Similarly with Range sizes)

Reviewable status: complete! 0 of 0 LGTMs obtained

pkg/config/zone.go, line 541 at r2 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

Ok, but what about callers that use GetSubzone instead of GetSubzoneForKeySuffix? Shouldn't that copy too?

Done.

pkg/config/zone_yaml.go, line 74 at r6 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

Is there something that makes it difficult to flip this around to Inherited so it matches the zone configs?

Done.

pkg/sql/set_zone_config.go, line 327 at r1 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

Oh, I see. I think this belongs up where partialSubzone is declared then. Can you do something like this?

var partialSubzone *config.Subzone
if index != nil {
	partialSubzone = partialZone.GetSubzone(uint32(index.ID), partition)
	if partialSubzone == nil {
		partialSubzone = &config.Subzone{Config: &config.ZoneConfig{}}
	}
}

Done.

pkg/sql/set_zone_config.go, line 273 at r3 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

"is not will be"

Done.

pkg/sql/set_zone_config.go, line 344 at r3 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

"Load"

Done.

petermattis · Oct 4, 2018

petermattis reviewed Oct 4, 2018

View changes

Skimmed through this PR. Generally looks good, but I'll defer to others for the final approval.

Reviewable status: complete! 0 of 0 LGTMs obtained

pkg/config/zone.go, line 538 at r7 (raw file):

		if s.IndexID == indexID && s.PartitionName == partition {
			copySubzone := s
			return &copySubzone

I believe this is equivalent to the previous code. &s was escaping to the heap, but that doesn't mean it was pointing into the Subzones slice. I prefer the new code, though, because it is obvious.

pkg/config/zone.go, line 556 at r7 (raw file):

		if (s.Key.Compare(keySuffix) <= 0) &&
			((s.EndKey == nil && bytes.HasPrefix(keySuffix, s.Key)) || s.EndKey.Compare(keySuffix) > 0) {
			copySubzone := z.Subzones[s.SubzoneIndex]

Is z.Subzones[s.SubzoneIndex] == s? I'd prefer if you used the same pattern above with copySubzone := s. Perhaps I don't understand some subtly here.

pkg/config/zone.proto, line 110 at r7 (raw file):

  // InheritedContraints specifies if value in the Constraints field was
  // pupulated by the user explicitly and not inherited from the zone's parent.

s/if value/if the value/g
s/pupulated/populated/g

pkg/config/zone.proto, line 124 at r7 (raw file):

  // InheritedContraints specifies if value in the Constraints field was
  // pupulated by the user explicitly and not inherited from the zone's parent.

Ditto about comment about grammar.

pkg/sql/zone_config.go, line 154 at r7 (raw file):

// InheritFromParent takes a pointer to a zone and hydrates its missing
// fields from its parent (given as another argument).
func InheritFromParent(cfg *config.ZoneConfig, parent config.ZoneConfig) {

I'm curious why this method is defined here. It doesn't seem to have anything sql specific. Looks to me like this could be a method on ZoneConfig, though perhaps I'm not seeing something preventing that. At least it feels like this belongs in the config package.

benesch · Oct 4, 2018

benesch reviewed Oct 4, 2018

View changes

once you've addressed Peter's comments and added the version check. There's also one (more) thing I think we should do before this lands re SHOW ZONE CONFIGURATIONS because I think it needs a last-minute backport to v2.1. It'll be much easier to do the backport before this goes in.

Great work on getting this done with the current state of the code, and thanks for bearing with me on this review. I'm note that I'm a bit nervous about test coverage and the ever-increasing complexity of this code. Will you have time for some refactoring after this lands? (I don't have anything specific in mind yet.) I also think we should write a roachtest that writes some zone configs in a v2.1 cluster, then upgrades half the nodes to v2.2 and verifies that all the nodes can read/write v2.1-style zone configs, then upgrades the other half to v2.2, bumps the cluster version, and verifies that

Reviewed 7 of 7 files at r7.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale)

pkg/config/zone.go, line 556 at r7 (raw file):

Previously, petermattis (Peter Mattis) wrote…

Is z.Subzones[s.SubzoneIndex] == s? I'd prefer if you used the same pattern above with copySubzone := s. Perhaps I don't understand some subtly here.

Yeah, s is a SubzoneSpan, while z.Subzones[s.SubzoneIndex] is a Subzone.

pkg/sql/set_zone_config.go, line 433 at r7 (raw file):

	// Write the partial zone configutation.
	// TODO(ridwanmsharif): If cluster version is below 2.2, just write the complete
	// zone config instead of the partial. That should be enough for backwards compatibility

Shoot, this needs to be done before this merges.

pkg/sql/zone_config.go, line 154 at r7 (raw file):

Previously, petermattis (Peter Mattis) wrote…

I'm curious why this method is defined here. It doesn't seem to have anything sql specific. Looks to me like this could be a method on ZoneConfig, though perhaps I'm not seeing something preventing that. At least it feels like this belongs in the config package.

Oh, great call. Agreed that this should move into config.

ridwanmsharif · Oct 11, 2018

ridwanmsharif reviewed Oct 11, 2018

View changes

Now that v2.1 has been created, progress can continue on this. I've written the new version change
and checked the behavior if the certain nodes on the cluster are from a previous version (tested with 2.1).
In which case, it falls back to the old mechanism of not cascading at all. In my sweep I didn't find any subtle
inconsistency with what we expect in behavior.

I've also modified the output of SHOW ALL ZONE CONFIGURATIONS to show the SQL command that will
get you to this exact state if applied. So they will not show any inherited fields now. All fields will be shown
if SHOW ZONE CONFIGURATION FOR ... is used for a specific zone.

Some work has to be done in the following PRs
to improve the visibility of this behavior (with a new column showing what config each field is using) and handling
the subtle validation issues that might still be a problem (as discussed in the RFC). Those changes are fairly easy,
but I think its best to leave them for other PRs. We also need to add a roachtest as @benesch described for this behavior change. This became increasingly complex and I would be down for some refactor of a few things in the ZoneConfig
logic soon. Its also worth mentioning that once we have primary key changes in crdb, leveraging that can make this so much saner.

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale)

pkg/config/zone.go, line 538 at r7 (raw file):

Previously, petermattis (Peter Mattis) wrote…

I believe this is equivalent to the previous code. &s was escaping to the heap, but that doesn't mean it was pointing into the Subzones slice. I prefer the new code, though, because it is obvious.

Ack.

pkg/config/zone.go, line 556 at r7 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

Yeah, s is a SubzoneSpan, while z.Subzones[s.SubzoneIndex] is a Subzone.

Ack.

pkg/config/zone.proto, line 110 at r7 (raw file):

Previously, petermattis (Peter Mattis) wrote…

s/if value/if the value/g
s/pupulated/populated/g

Done.

pkg/config/zone.proto, line 124 at r7 (raw file):

Previously, petermattis (Peter Mattis) wrote…

Ditto about comment about grammar.

Done.

pkg/sql/set_zone_config.go, line 433 at r7 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

Shoot, this needs to be done before this merges.

Done.

pkg/sql/zone_config.go, line 154 at r7 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

Oh, great call. Agreed that this should move into config.

Done.

benesch · Oct 12, 2018

benesch approved these changes Oct 12, 2018

View changes

with nits

Reviewed 1 of 15 files at r6, 29 of 29 files at r8.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale)

pkg/config/system.go, line 296 at r8 (raw file):
I don't think you need to take the address of subzone.Config. Go should do that automatically.

A method call x.m() is valid if the method set of (the type of) x contains m and the argument list can be assigned to the parameter list of m. If x is addressable and &x's method set contains m, x.m() is shorthand for (&x).m():

—https://golang.org/ref/spec#Calls

Am I crazy?

pkg/server/updates_test.go, line 578 at r8 (raw file):

		"server.time_until_store_dead":             "1m30s",
		"trace.debug.enable":                       "false",
		"version":                                  cluster.BinaryServerVersion.String(),

Was this supposed to get removed?

pkg/settings/cluster/cockroach_versions.go, line 293 at r8 (raw file):

		Version: roachpb.Version{Major: 2, Minor: 1},
	},
	{ // VersionCascadingZoneConfigs is https://github.com/cockroachdb/cockroach/pull/30611.

This comment should be on the line beneath the brace for consistency with the other entries in the slice.

pkg/sql/zone_config.go, line 212 at r8 (raw file):

					(&subzone.Config).InheritFromParent(indexSubzone.Config)
				}
				(&subzone.Config).InheritFromParent(*zone)

I think you can elide the explicit address-of here too.

petermattis · Oct 12, 2018

petermattis approved these changes Oct 12, 2018

View changes

Reviewable status: complete! 1 of 0 LGTMs obtained

ridwanmsharif · Oct 12, 2018

ridwanmsharif reviewed Oct 12, 2018

View changes

Fixed the nits, letting this change fly now then 🎆

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale)

pkg/config/system.go, line 296 at r8 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

I don't think you need to take the address of subzone.Config. Go should do that automatically.

A method call x.m() is valid if the method set of (the type of) x contains m and the argument list can be assigned to the parameter list of m. If x is addressable and &x's method set contains m, x.m() is shorthand for (&x).m():

—https://golang.org/ref/spec#Calls

Am I crazy?

Done. No you're right :)

pkg/settings/cluster/cockroach_versions.go, line 293 at r8 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

This comment should be on the line beneath the brace for consistency with the other entries in the slice.

Done.

pkg/sql/zone_config.go, line 212 at r8 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

I think you can elide the explicit address-of here too.

Done.

pkg/server/updates_test.go, line 578 at r8 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

Was this supposed to get removed?

Done.

bors r+

ridwanmsharif · Oct 12, 2018

bors r+

Build succeeded

GitHub CI (Cockroach)

craig · Oct 12, 2018

Build succeeded

GitHub CI (Cockroach)

ridwanmsharif added the do-not-merge label Sep 25, 2018

ridwanmsharif requested review from cockroachdb/build-prs as code owners Sep 25, 2018

ridwanmsharif removed request for cockroachdb/core-prs Sep 25, 2018

ridwanmsharif requested a review from cockroachdb/cli-prs as a code owner Sep 25, 2018

ridwanmsharif removed the request for review from cockroachdb/cli-prs Sep 25, 2018

ridwanmsharif changed the title from [WIP] sql, config: make zone configs cascade to sql, config: make zone configs cascade Sep 26, 2018

ridwanmsharif removed the do-not-merge label Sep 26, 2018

ridwanmsharif requested a review from cockroachdb/sql-rest-prs as a code owner Sep 27, 2018

ridwanmsharif removed the request for review from cockroachdb/sql-rest-prs Sep 27, 2018

petermattis requested review from a-robinson and benesch Oct 1, 2018

ridwanmsharif requested review from cockroachdb/distsql-prs as code owners Oct 2, 2018

ridwanmsharif removed request for cockroachdb/distsql-prs Oct 2, 2018

ridwanmsharif requested a review from benesch Oct 11, 2018

craig bot merged commit 55b4146 into cockroachdb:master Oct 12, 2018
3 checks passed

3 checks passed

GitHub CI (Cockroach) TeamCity build finished
Details

bors Build succeeded
Details

license/cla Contributor License Agreement is signed.
Details

ridwanmsharif deleted the ridwanmsharif:cascading-zone-configs branch Oct 12, 2018

cockroachdb/cockroach

sql, config: make zone configs cascade #30611

ridwanmsharif commented Sep 25, 2018 •

edited

ridwanmsharif added the do-not-merge label Sep 25, 2018

ridwanmsharif requested review from cockroachdb/build-prs as code owners Sep 25, 2018

This comment has been minimized.

cockroach-teamcity commented Sep 25, 2018

ridwanmsharif removed request for cockroachdb/core-prs Sep 25, 2018

ridwanmsharif requested a review from cockroachdb/cli-prs as a code owner Sep 25, 2018

ridwanmsharif removed the request for review from cockroachdb/cli-prs Sep 25, 2018

ridwanmsharif changed the title from [WIP] sql, config: make zone configs cascade to sql, config: make zone configs cascade Sep 26, 2018

ridwanmsharif removed the do-not-merge label Sep 26, 2018

ridwanmsharif requested a review from cockroachdb/sql-rest-prs as a code owner Sep 27, 2018

ridwanmsharif removed the request for review from cockroachdb/sql-rest-prs Sep 27, 2018

petermattis requested review from a-robinson and benesch Oct 1, 2018

benesch reviewed Oct 1, 2018

View changes

ridwanmsharif reviewed Oct 1, 2018

View changes

ridwanmsharif reviewed Oct 1, 2018

View changes

benesch reviewed Oct 2, 2018

View changes

ridwanmsharif requested review from cockroachdb/distsql-prs as code owners Oct 2, 2018

ridwanmsharif reviewed Oct 2, 2018

View changes

ridwanmsharif removed request for cockroachdb/distsql-prs Oct 2, 2018

benesch reviewed Oct 3, 2018

View changes

ridwanmsharif reviewed Oct 4, 2018

View changes

petermattis reviewed Oct 4, 2018

View changes

benesch reviewed Oct 4, 2018

View changes

ridwanmsharif reviewed Oct 11, 2018

View changes

ridwanmsharif requested a review from benesch Oct 11, 2018

ridwanmsharif added some commits Sep 20, 2018

benesch approved these changes Oct 12, 2018

View changes

petermattis approved these changes Oct 12, 2018

View changes

ridwanmsharif reviewed Oct 12, 2018

View changes

This comment has been minimized.

ridwanmsharif commented Oct 12, 2018

craig bot pushed a commit that referenced this pull request Oct 12, 2018

This comment has been minimized.

craig bot commented Oct 12, 2018

craig bot merged commit `55b4146` into cockroachdb:master Oct 12, 2018
3 checks passed

3 checks passed

ridwanmsharif deleted the ridwanmsharif:cascading-zone-configs branch Oct 12, 2018

cockroachdb/cockroach

Join GitHub today

sql, config: make zone configs cascade #30611

Conversation

ridwanmsharif commented Sep 25, 2018 • edited

ridwanmsharif added the do-not-merge label Sep 25, 2018

ridwanmsharif requested review from cockroachdb/build-prs as code owners Sep 25, 2018

This comment has been minimized.

cockroach-teamcity Sep 25, 2018

cockroach-teamcity commented Sep 25, 2018

ridwanmsharif removed request for cockroachdb/core-prs Sep 25, 2018

ridwanmsharif requested a review from cockroachdb/cli-prs as a code owner Sep 25, 2018

ridwanmsharif removed the request for review from cockroachdb/cli-prs Sep 25, 2018

ridwanmsharif changed the title from [WIP] sql, config: make zone configs cascade to sql, config: make zone configs cascade Sep 26, 2018

ridwanmsharif removed the do-not-merge label Sep 26, 2018

ridwanmsharif requested a review from cockroachdb/sql-rest-prs as a code owner Sep 27, 2018

ridwanmsharif removed the request for review from cockroachdb/sql-rest-prs Sep 27, 2018

petermattis requested review from a-robinson and benesch Oct 1, 2018

benesch reviewed Oct 1, 2018 View changes

ridwanmsharif reviewed Oct 1, 2018 View changes

ridwanmsharif reviewed Oct 1, 2018 View changes

benesch reviewed Oct 2, 2018 View changes

ridwanmsharif requested review from cockroachdb/distsql-prs as code owners Oct 2, 2018

ridwanmsharif reviewed Oct 2, 2018 View changes

ridwanmsharif removed request for cockroachdb/distsql-prs Oct 2, 2018

benesch reviewed Oct 3, 2018 View changes

ridwanmsharif reviewed Oct 4, 2018 View changes

petermattis reviewed Oct 4, 2018 View changes

benesch reviewed Oct 4, 2018 View changes

ridwanmsharif reviewed Oct 11, 2018 View changes

ridwanmsharif requested a review from benesch Oct 11, 2018

ridwanmsharif added some commits Sep 20, 2018

benesch approved these changes Oct 12, 2018 View changes

petermattis approved these changes Oct 12, 2018 View changes

ridwanmsharif reviewed Oct 12, 2018 View changes

This comment has been minimized.

ridwanmsharif Oct 12, 2018

ridwanmsharif commented Oct 12, 2018

craig bot pushed a commit that referenced this pull request Oct 12, 2018

This comment has been minimized.

craig bot Oct 12, 2018

Build succeeded

craig bot commented Oct 12, 2018

Build succeeded

Hide details View details craig bot merged commit 55b4146 into cockroachdb:master Oct 12, 2018 3 checks passed

3 checks passed

ridwanmsharif deleted the ridwanmsharif:cascading-zone-configs branch Oct 12, 2018

ridwanmsharif commented Sep 25, 2018 •

edited

benesch reviewed Oct 1, 2018

View changes

ridwanmsharif reviewed Oct 1, 2018

View changes

ridwanmsharif reviewed Oct 1, 2018

View changes

benesch reviewed Oct 2, 2018

View changes

ridwanmsharif reviewed Oct 2, 2018

View changes

benesch reviewed Oct 3, 2018

View changes

ridwanmsharif reviewed Oct 4, 2018

View changes

petermattis reviewed Oct 4, 2018

View changes

benesch reviewed Oct 4, 2018

View changes

ridwanmsharif reviewed Oct 11, 2018

View changes

benesch approved these changes Oct 12, 2018

View changes

petermattis approved these changes Oct 12, 2018

View changes

ridwanmsharif reviewed Oct 12, 2018

View changes

craig bot merged commit `55b4146` into cockroachdb:master Oct 12, 2018
3 checks passed