Skip to content

Conversation

lnhsingh
Copy link
Contributor

  • Change warning to info callout. Changes to a zone will now cascade
  • Add new COPY FROM PARENT value that allows a child zone to copy values
    from a parent zone. When this is used, changes will no longer cascade
  • Update SQL diagrams

Closes #4062, #4255.

- Change warning to info that changes to a zone will cascade
- Add new COPY FROM PARENT value that allows a child zone to copy values 
from a parent zone
- Update SQL diagrams

Closes #4062, #4255.
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@cockroach-teamcity
Copy link
Member

@cockroach-teamcity
Copy link
Member

Copy link
Contributor

@knz knz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The explanatory phrasing is good, but can you verify this is also applicable to 2.1? I have a vague recollection this was specific to 19.1.

Reviewed 15 of 16 files at r1, 2 of 2 files at r2.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @Amruta-Ranade)

Copy link
Contributor Author

@lnhsingh lnhsingh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TFTR! Confirmed- COPY FROM PARENT isn't in v2.1, but zone configs cascade.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @Amruta-Ranade)

@cockroach-teamcity
Copy link
Member

@lnhsingh lnhsingh requested a review from jseldess March 4, 2019 03:59
Copy link
Contributor

@jseldess jseldess left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed in person, I'm still pretty confused about the behavior of zone configs after this change. The new note about COPY FROM PARENT is clear, but in conjunction with the note about the .default replication zone, it's not clear how this all interacts.

My sense (and I could be wrong) is that now, when you create a replication zone, it inherits the values from the .default replication zone by default. If you want it to inherit a value from its direct parent instead, you can use COPY FROM PARENT. However, in that case, if the value is changed in the parent replication zone after the child is created, the child doesn't inherit that change.

@knz or @bdarnell, can you help validate the overall logic here?

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @jseldess)

@knz
Copy link
Contributor

knz commented Mar 4, 2019

Yes I think your explanation is valid.

Just be minfdul that .default is overall less important. New zones inherit from their parent which is not necessarily .default (eg a partition zone inherits from the table).

There are two possible "values" stored in a zone cfg:

  • value is absent: the parent zonecfg applies, or the grandparent if the parent setting is also absent, etc, up to .default
  • value is present: the value applies.

To achieve these two cases you can use the following methods:

  • Value becomes present:

    • when set explicitly (SET ...)
    • when COPY FROM PARENT is used during zonecfg creation
    • when COPY FROM PARENT is used after zonecfg creation
  • Value becomes absent:

    • when omitted during zonecfg creation
    • when DROP is used
    • (not documented, not supported, preserved for compatibility with 2.0/2.1) when value set to NULL

Maybe this presentation (with a table) could be a good complement to the notes already added in the PR.

@@ -1 +1 @@
Changes to the [`.default` cluster-wide replication zone](configure-replication-zones.html#edit-the-default-replication-zone) are not automatically applied to existing replication zones, including pre-configured zones for important system ranges that must remain available for the cluster as a whole to remain available. The zones for these system ranges have an initial replication factor of 5 to make them more resilient to node failure. However, if you increase the `.default` zone's replication factor above 5, consider [increasing the replication factor for important system ranges](configure-replication-zones.html#create-a-replication-zone-for-a-system-range) as well.
Changes to the [`.default` cluster-wide replication zone](configure-replication-zones.html#edit-the-default-replication-zone) are automatically applied to existing replication zones, including pre-configured zones for important system ranges that must remain available for the cluster as a whole to remain available. The zones for these system ranges have an initial replication factor of 5 to make them more resilient to node failure. However, if you increase the `.default` zone's replication factor above 5, consider [increasing the replication factor for important system ranges](configure-replication-zones.html#create-a-replication-zone-for-a-system-range) as well.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is unclear - the second sentence is an exception to the first (and it's not the only one - if you change range_{min,max}_bytes or gc.ttl, those also won't be applied to the system zones because they have default values for those fields).

Do we really need this callout any more? It was important when even system zones defaulted to 3 replicas. Now that they default to 5, it's saying that if you set your default replication factor to 7 or more you may need to do the same to the system zones, but we don't really recommend running with more than 5 replicas.

I think there should be a mention on the main page for zone config docs that says that the system zones have their own zone configs that may override settings from .default, but we don't need to copy this warning everywhere zone configs are mentioned.

@@ -1 +1 @@
Changes to the [`.default` cluster-wide replication zone](configure-replication-zones.html#edit-the-default-replication-zone) are not automatically applied to existing replication zones, including pre-configured zones for important system ranges that must remain available for the cluster as a whole to remain available. The zones for these system ranges have an initial replication factor of 5 to make them more resilient to node failure. However, if you increase the `.default` zone's replication factor above 5, consider [increasing the replication factor for important system ranges](configure-replication-zones.html#create-a-replication-zone-for-a-system-range) as well.
<span class="version-tag">New in v2.1:</span> Changes to the [`.default` cluster-wide replication zone](configure-replication-zones.html#edit-the-default-replication-zone) are automatically applied to existing replication zones, including pre-configured zones for important system ranges that must remain available for the cluster as a whole to remain available. The zones for these system ranges have an initial replication factor of 5 to make them more resilient to node failure. However, if you increase the `.default` zone's replication factor above 5, consider [increasing the replication factor for important system ranges](configure-replication-zones.html#create-a-replication-zone-for-a-system-range) as well.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is true - as far as I can see cascading zone configs are new in 19.1.

Copy link
Contributor Author

@lnhsingh lnhsingh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a table to further explain this. @knz, when you say "value becomes absent when DROP is used", do you mean RESET?

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @bdarnell)


_includes/v19.1/known-limitations/system-range-replication.md, line 1 at r3 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

This is unclear - the second sentence is an exception to the first (and it's not the only one - if you change range_{min,max}_bytes or gc.ttl, those also won't be applied to the system zones because they have default values for those fields).

Do we really need this callout any more? It was important when even system zones defaulted to 3 replicas. Now that they default to 5, it's saying that if you set your default replication factor to 7 or more you may need to do the same to the system zones, but we don't really recommend running with more than 5 replicas.

I think there should be a mention on the main page for zone config docs that says that the system zones have their own zone configs that may override settings from .default, but we don't need to copy this warning everywhere zone configs are mentioned.

Added to configure-replication-zones.md and removed this callout elsewhere


_includes/v2.1/known-limitations/system-range-replication.md, line 1 at r3 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

I don't think this is true - as far as I can see cascading zone configs are new in 19.1.

Done.

@cockroach-teamcity
Copy link
Member

@lnhsingh
Copy link
Contributor Author

lnhsingh commented Mar 6, 2019

@jseldess + @knz and/or @bdarnell, can you do another pass on this please?

Copy link
Contributor

@bdarnell bdarnell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @bdarnell, @knz, and @lhirata)


_includes/v19.1/zone-configs/variables.md, line 12 at r4 (raw file):

Unless a value is set, new zone configurations will inherit their values from their parent zone (e.g., a partition zone inherits from the table zone), which is not necessarily `.default`.

If a variable is set to `COPY FROM PARENT` (e.g., `range_max_bytes = COPY FROM PARENT`), the variable will copy its value from its parent [replication zone](configure-replication-zones.html). However, if the variable in the parent replication zone is changed after the child replication zone is copied, the change will not be reflected in the child zone.

When discussing COPY FROM PARENT, we should probably talk about why it's sometimes required: it's an error to set range_max_bytes to a lower value than range_min_bytes, so to avoid confusion we require that when one is set, the other must be set too. COPY FROM PARENT is a convenience so the user doesn't have to look up the parent's current value.


_includes/v19.1/zone-configs/variables.md, line 14 at r4 (raw file):

If a variable is set to `COPY FROM PARENT` (e.g., `range_max_bytes = COPY FROM PARENT`), the variable will copy its value from its parent [replication zone](configure-replication-zones.html). However, if the variable in the parent replication zone is changed after the child replication zone is copied, the change will not be reflected in the child zone.

There are two "value settings" per variable in a zone configuration:

"Value settings" sounds strange to me. This seems like a lot of words (and visual weight due to the table) to describe the idea that values that are not set at a particular level will be inherited from the parent.

Copy link
Contributor Author

@lnhsingh lnhsingh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @bdarnell and @knz)


_includes/v19.1/zone-configs/variables.md, line 12 at r4 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

When discussing COPY FROM PARENT, we should probably talk about why it's sometimes required: it's an error to set range_max_bytes to a lower value than range_min_bytes, so to avoid confusion we require that when one is set, the other must be set too. COPY FROM PARENT is a convenience so the user doesn't have to look up the parent's current value.

Done.


_includes/v19.1/zone-configs/variables.md, line 14 at r4 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

"Value settings" sounds strange to me. This seems like a lot of words (and visual weight due to the table) to describe the idea that values that are not set at a particular level will be inherited from the parent.

Done.

@cockroach-teamcity
Copy link
Member

Copy link
Contributor

@bdarnell bdarnell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @bdarnell and @knz)

Copy link
Contributor

@jseldess jseldess left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Thanks, @lhirata!

Reviewable status: :shipit: complete! 2 of 0 LGTMs obtained (waiting on @bdarnell and @knz)

@lnhsingh
Copy link
Contributor Author

lnhsingh commented Mar 7, 2019

TFTRs!

@lnhsingh lnhsingh merged commit 7c4a67b into master Mar 7, 2019
@lnhsingh lnhsingh deleted the zone-config branch March 7, 2019 21:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants