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

ui: show correct zone config #59196

Merged
merged 1 commit into from Jan 28, 2021

Conversation

nkodali
Copy link
Collaborator

@nkodali nkodali commented Jan 20, 2021

Previously on the table detail page in DB Console,
the default zone config for the db was shown, rather
than any table level settings. Additionally,
constraints and lease preferences were not being
serialized properly, displaying as
"...Object object..." This fixes both of those bugs.

The display for the zone configuration statement is
additionally updated to show the actual SQL statement
to replicate the zone config in the SQL shell.
Previously an invalid statement "CONFIGURE ZONE USING..."
was displayed.

Resolves #57896.
See also: https://github.com/cockroachlabs/support/issues/737.
See also: https://github.com/cockroachlabs/support/issues/727.

Release note (bug fix): Fixed a bug introduced in v20.1 in
DB Console where incorrect zone configuration values were
shown on the table details page and constraints and lease
preferences were not displayed.

Release note (ui change): Updates the table details page
to show table specific zone configuration values when set,
show constraints and lease preferences, and display
a valid statement to re-configure zone configuration
for that table.

@nkodali nkodali requested a review from a team as a code owner January 20, 2021 15:51
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Collaborator

@dhartunian dhartunian left a comment

Choose a reason for hiding this comment

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

Thank you for making this fix! I tested with the other support issue we had and it does fix it as well! Just added a note to also fix some of the zone config serialization while you're at it.

Run eslint and make sure Goland is configured to autoformat w/ it.

I'd add a second release note for db console. My understanding is that it's fine to have multiple release note entries if your PR covers multiple categories.

Reviewed 1 of 3 files at r1.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @nkodali)


pkg/ui/src/views/shared/components/sql/highlight.tsx, line 65 at r1 (raw file):

        <br />
        <span className="hljs-label">constraints = ['</span>
        <span className="hljs-built_in">{String(zoneConfig.constraints)}</span>

Could we update this to print the proper constraints instead of constraints = ['[object Object],[object Object]'],. There are only two fields here and it should look like:

demo@127.0.0.1:26257/movr> SHOW ZONE CONFIGURATION FOR TABLE users;
    target    |                           raw_config_sql
--------------+---------------------------------------------------------------------
  TABLE users | ALTER TABLE users CONFIGURE ZONE USING
              |     range_min_bytes = 16777216,
              |     range_max_bytes = 67108864,
              |     gc.ttlseconds = 9000,
              |     num_replicas = 3,
              |     constraints = '{+region=us-east1: 1, +region=us-west1: 1}',
              |     lease_preferences = '[[+region=us-east1], [+region=us-west1]]'
(1 row)

pkg/ui/src/views/shared/components/sql/highlight.tsx, line 70 at r1 (raw file):

        <span className="hljs-label">lease_preferences = [['</span>
        <span className="hljs-built_in">
          {String(zoneConfig.lease_preferences)}

Same comment here as above, would be nice to get the serialization bug out of the way as well here. Example up above.

@nkodali nkodali force-pushed the 20210115-zone-config-db-page-bug branch from a52a025 to 5198f47 Compare January 27, 2021 14:43
@nkodali nkodali requested review from a team as code owners January 27, 2021 14:43
@nkodali nkodali force-pushed the 20210115-zone-config-db-page-bug branch from 5198f47 to f04322f Compare January 27, 2021 15:20
@dhartunian dhartunian self-requested a review January 27, 2021 15:59
Copy link
Collaborator

@dhartunian dhartunian 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 for doing this!

Reviewed 6 of 8 files at r2.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @dhartunian and @nkodali)


pkg/server/admin.go, line 612 at r2 (raw file):

		const rawConfigSqlColName = "raw_config_sql"
		if len(rows) != 1 {
			return nil, s.serverErrorf("show zone configuration not available.")

Is it possible that we could have a perfectly valid table with an empty zone config?

@nkodali nkodali force-pushed the 20210115-zone-config-db-page-bug branch from f04322f to de3adb6 Compare January 27, 2021 20:01
Copy link
Collaborator Author

@nkodali nkodali left a comment

Choose a reason for hiding this comment

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

TFTR. Updated to more gracefully handle empty zone config.

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


pkg/server/admin.go, line 612 at r2 (raw file):

Previously, dhartunian (David Hartunian) wrote…

Is it possible that we could have a perfectly valid table with an empty zone config?

My limited understanding of zone configs is that there is always a default. Despite that, I think it's safer to return empty string in the case this query gives no results, rather than a server error. Updated to do that.


pkg/ui/src/views/shared/components/sql/highlight.tsx, line 65 at r1 (raw file):

Previously, dhartunian (David Hartunian) wrote…

Could we update this to print the proper constraints instead of constraints = ['[object Object],[object Object]'],. There are only two fields here and it should look like:

demo@127.0.0.1:26257/movr> SHOW ZONE CONFIGURATION FOR TABLE users;
    target    |                           raw_config_sql
--------------+---------------------------------------------------------------------
  TABLE users | ALTER TABLE users CONFIGURE ZONE USING
              |     range_min_bytes = 16777216,
              |     range_max_bytes = 67108864,
              |     gc.ttlseconds = 9000,
              |     num_replicas = 3,
              |     constraints = '{+region=us-east1: 1, +region=us-west1: 1}',
              |     lease_preferences = '[[+region=us-east1], [+region=us-west1]]'
(1 row)

Done.


pkg/ui/src/views/shared/components/sql/highlight.tsx, line 70 at r1 (raw file):

Previously, dhartunian (David Hartunian) wrote…

Same comment here as above, would be nice to get the serialization bug out of the way as well here. Example up above.

Done.

@nkodali nkodali force-pushed the 20210115-zone-config-db-page-bug branch from de3adb6 to f83ef51 Compare January 27, 2021 20:41
@dhartunian
Copy link
Collaborator

You've got a frontend TS error:

16:07:00
    src/views/databases/containers/databaseTables/databaseTables.fixtures.ts(47,3): error TS2322: Type '{ detailsAndStatsLoaded: () => true; name: string; id: number; numColumns: number; numIndices: number; rangeCount: number; createStatement: string; grants: { user: string; privileges: string[]; }[]; numReplicas: number; mvccSize: { ...; }; physicalSize: number; }[]' is not assignable to type 'TableInfo[]'.
16:07:00
      Property 'configureZoneStatement' is missing in type '{ detailsAndStatsLoaded: () => true; name: string; id: number; numColumns: number; numIndices: number; rangeCount: number; createStatement: string; grants: { user: string; privileges: string[]; }[]; numReplicas: number; mvccSize: { ...; }; physicalSize: number; }' but required in type 'TableInfo'.

Previously on the table detail page in DB Console,
the default zone config for the db was shown, rather
than any table level settings. Additionally,
constraints and lease preferences were not being
serialized properly, displaying as
"...Object object..." This fixes both of those bugs.

The display for the zone configuration statement is
additionally updated to show the actual SQL statement
to replicate the zone config in the SQL shell.
Previously an invalid statement "CONFIGURE ZONE USING..."
was displayed.

Resolves cockroachdb#57896.
See also: cockroachlabs/support#737.
See also: cockroachlabs/support#727.

Release note (bug fix): Fixed a bug introduced in v20.1 in
DB Console where incorrect zone configuration values were
shown on the table details page and constraints and lease
preferences were not displayed.

Release note (ui change): Updates the table details page
to show table specific zone configuration values when set,
show constraints and lease preferences, and display
a valid statement to re-configure zone configuration
for that table.
@nkodali nkodali force-pushed the 20210115-zone-config-db-page-bug branch from f83ef51 to c40ec31 Compare January 28, 2021 01:03
@nkodali
Copy link
Collaborator Author

nkodali commented Jan 28, 2021

bors r=dhartunian

@craig
Copy link
Contributor

craig bot commented Jan 28, 2021

Build failed:

@nkodali
Copy link
Collaborator Author

nkodali commented Jan 28, 2021

bors r=dhartunian

@craig
Copy link
Contributor

craig bot commented Jan 28, 2021

Build succeeded:

@craig craig bot merged commit eddbc07 into cockroachdb:master Jan 28, 2021
@nkodali nkodali deleted the 20210115-zone-config-db-page-bug branch November 17, 2021 19:02
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.

DB Console: Databases page is showing incorrect zone configuration values
3 participants