Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

config: propagate zone config info for non-gossiped system tables #39638

Merged

Conversation

ajwerner
Copy link
Contributor

Before this PR we would allow setting zone configurations on system tables but
they would not propagate because the GetZoneConfig function would return the
zone config for the entire system database. This leaves us in a still weird
situation where some system tables will ignore their zone configs but not
others.

Fixes #39605.

Release note (bug fix): Propagate zone configuration to non-gossiped system
tables.

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@ajwerner
Copy link
Contributor Author

ajwerner commented Aug 13, 2019

I'm happy to write an end-to-end test too if the reviewer would like it. I did verify manually that this change propagates MaxRangeBytes.

@ajwerner ajwerner force-pushed the ajwerner/fix-jobs-table-zone-configs branch from 31417e6 to 1584c0e Compare August 13, 2019 21:38
@ajwerner
Copy link
Contributor Author

This still breaks some tests, let me fix them and I'll let you know when it's RFAL.

@ajwerner ajwerner force-pushed the ajwerner/fix-jobs-table-zone-configs branch 2 times, most recently from 52e1329 to 7a33e5e Compare August 14, 2019 18:51
Copy link
Contributor Author

@ajwerner ajwerner left a comment

Choose a reason for hiding this comment

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

Alright, figured it out. I needed to make sure to translate pseudo-table ids into the system database ID. RFAL.

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

Before this PR we would allow setting zone configurations on system tables but
they would not propagate because the GetZoneConfig function would return the
zone config for the entire system database. This leaves us in a still weird
situation where some system tables will ignore their zone configs but not
others.

Fixes cockroachdb#39605.

Release note (bug fix): Propagate zone configuration to non-gossiped system
tables.
@ajwerner ajwerner force-pushed the ajwerner/fix-jobs-table-zone-configs branch from 7a33e5e to ea56965 Compare August 15, 2019 13:25
Copy link
Member

@nvanbenschoten nvanbenschoten left a comment

Choose a reason for hiding this comment

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

:lgtm: nice find.

Reviewed 4 of 4 files at r1.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained

@ajwerner
Copy link
Contributor Author

bors r=nvanbenschoten

craig bot pushed a commit that referenced this pull request Aug 15, 2019
39638: config: propagate zone config info for non-gossiped system tables r=nvanbenschoten a=ajwerner

Before this PR we would allow setting zone configurations on system tables but
they would not propagate because the GetZoneConfig function would return the
zone config for the entire system database. This leaves us in a still weird
situation where some system tables will ignore their zone configs but not
others.

Fixes #39605.

Release note (bug fix): Propagate zone configuration to non-gossiped system
tables.

Co-authored-by: Andrew Werner <ajwerner@cockroachlabs.com>
@craig
Copy link
Contributor

craig bot commented Aug 15, 2019

Build succeeded

@craig craig bot merged commit ea56965 into cockroachdb:master Aug 15, 2019
ajwerner added a commit to ajwerner/cockroach that referenced this pull request Apr 23, 2020
The PublicSchemaID was introduced during the work for temp tables and the
associated migration of the namespace table. The chosen constantly was
erroneously put into the `PseudoTableIDs` which is used to create split
points and control zone configs for low-level constructs like meta ranges,
liveness range, timeseries, etc.

This commit also fixed non-sense I introduced in cockroachdb#39638 while trying to
enable real system tables to have their own zone configs (as opposed to
just being controlled by the system zone config). All of the pseudo-table
ids had corresponding filters below.

This change should get backported as far back as 19.1.

Release note (bug fix): Fix bug where an extra split was created in the
keyspace which does not correspond to user data.
ajwerner added a commit to ajwerner/cockroach that referenced this pull request Jul 1, 2020
…o IDs

The PublicSchemaID was introduced during the work for temp tables and the
associated migration of the namespace table. The chosen constantly was
erroneously put into the PseudoTableIDs which is used to create split
points and control zone configs for low-level constructs like meta ranges,
liveness range, timeseries, etc.

This commit also fixed non-sense I introduced in cockroachdb#39638 while trying to
enable real system tables to have their own zone configs (as opposed to
just being controlled by the system zone config). All of the pseudo-table
ids had corresponding filters below.

This change should get backported as far back as 19.1.

Release note (bug fix): Fix bug where an extra split was created in the
keyspace which does not correspond to user data which could lead to a
report of an under-replicated range in mixed-version 19.2-20.1 clusters.
ajwerner added a commit to ajwerner/cockroach that referenced this pull request Jul 2, 2020
The PublicSchemaID was introduced during the work for temp tables and the
associated migration of the namespace table. The chosen constantly was
erroneously put into the PseudoTableIDs which is used to create split
points and control zone configs for low-level constructs like meta ranges,
liveness range, timeseries, etc.

This commit also fixed non-sense I introduced in cockroachdb#39638 while trying to
enable real system tables to have their own zone configs (as opposed to
just being controlled by the system zone config). All of the pseudo-table
ids had corresponding filters below.

This change should get backported as far back as 19.1.

Release note (bug fix): Fix bug where an extra split was created in the
keyspace which does not correspond to user data that could appear to
be under-replicated in mixed-version clusters due to conflicting
default zone configrations.
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.

Not able to modify the zone configuration for the system.jobs table directly
4 participants