sql: Fix SHOW ZONE CONFIGURATIONS with very long constraints #69470
Conversation
|
This change is |
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @ajstorm and @rafiss)
pkg/sql/show_zone_config.go, line 178 at r1 (raw file):
// zoneConfigToSQL pretty prints a zone configuration as a SQL string. func zoneConfigToSQL(zs *tree.ZoneSpecifier, zone *zonepb.ZoneConfig) (string, error) { yaml.FutureLineWrap()
nit: comment
pkg/sql/logictest/logic.go, line 687 at r1 (raw file):
}, { name: "multiregion-3node-3superlongregions",
Do we anticipate using this configuration for any other tests? I'm not sure what the rule of thumb is when adding these one off configurations vs. writing a test that just uses a test cluster.
ajstorm
force-pushed
the
ajstorm-very-long-zc-2
branch
from
a95991d
to
eb1dee0
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/sql/show_zone_config.go, line 178 at r1 (raw file):
Previously, arulajmani (Arul Ajmani) wrote…
nit: comment
Good call. Added.
pkg/sql/logictest/logic.go, line 687 at r1 (raw file):
Previously, arulajmani (Arul Ajmani) wrote…
Do we anticipate using this configuration for any other tests? I'm not sure what the rule of thumb is when adding these one off configurations vs. writing a test that just uses a test cluster.
I was thinking that we might want to down the road, but I wanted to get this small test in first to confirm that the specific case I was hoping to address is covered.
There was a problem hiding this comment.
Reviewable status:
pkg/sql/logictest/logic.go, line 687 at r1 (raw file):
Previously, ajstorm (Adam Storm) wrote…
I was thinking that we might want to down the road, but I wanted to get this small test in first to confirm that the specific case I was hoping to address is covered.
That makes sense. I'd have a mild preference for making these "real" looking region names and having (say) 9 different regions in the configuration. Then, we could hit the character limit by adding more regions to the database instead. Such a configuration seems more generally useful as well.
There was a problem hiding this comment.
Reviewable status:
pkg/sql/logictest/logic.go, line 687 at r1 (raw file):
Previously, arulajmani (Arul Ajmani) wrote…
That makes sense. I'd have a mild preference for making these "real" looking region names and having (say) 9 different regions in the configuration. Then, we could hit the character limit by adding more regions to the database instead. Such a configuration seems more generally useful as well.
Fair point, but in the interest of expediency, I'll put this one off until we expand the scope of the test cases.
There was a problem hiding this comment.
TFTR!
bors r=arulajmani
Reviewable status:
|
This PR was included in a batch that was canceled, it will be automatically retried |
|
Build failed (retrying...): |
|
This PR was included in a batch that was canceled, it will be automatically retried |
|
Have to be away from the computer tonight and all day tomorrow and don't want this to prevent others from merging (on the off chance that this hits issues). Cancelling for now and will try to catch a train on Sunday. Good luck fellow borsers! bors r- |
|
Canceled. |
|
bors r=arulajmani |
|
Merge conflict. |
Previously, in the presence of very long constraints fields, SHOW ZONE CONFIGURATIONS would output the constraints with `\n` characters mixed in. This was due to the fact that the yaml.v2 library contained an 80 character line limit. We recently pulled in some commits to our fork of the yaml library which allows the line length to be configurable. With that change, we can now configure the line length to be unlimited in the case where we're showing the zone configuration, and get around the ugliness of the `\n` characters. Release note (sql change): Fixes a bug in SHOW ZONE CONFIGURATIONS where long constraints fields may show `\n` characters. Release justification: Low risk change to existing functionality.
ajstorm
force-pushed
the
ajstorm-very-long-zc-2
branch
from
eb1dee0
to
cc2279d
Compare
|
bors r=arulajmani |
|
Build succeeded: |
Previously, in the presence of very long constraints fields, SHOW ZONE
CONFIGURATIONS would output the constraints with
\ncharacters mixed in. Thiswas due to the fact that the yaml.v2 library contained an 80 character line
limit. We recently pulled in some commits to our fork of the yaml library which
allows the line length to be configurable. With that change, we can now
configure the line length to be unlimited in the case where we're showing the
zone configuration, and get around the ugliness of the
\ncharacters.Release note (sql change): Fixes a bug in SHOW ZONE CONFIGURATIONS where long
constraints fields may show
\ncharacters.Release justification: Low risk change to existing functionality.