-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
cluster-ui: move table details endpoint to use sql-over-http #98326
Conversation
18476bf
to
a534b21
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @THardy98)
pkg/ui/workspaces/cluster-ui/src/api/tableDetailsApi.ts
line 289 at r1 (raw file):
// and not empty. if ( txn_result.rows[0].database_zone_config_bytes &&
nit: txn_result.rows[0]
seems to be accessed a lot in this method. Why not set it to a const and use that everywhere to avoid the long lines of code?
pkg/ui/workspaces/cluster-ui/src/api/tableDetailsApi.ts
line 292 at r1 (raw file):
txn_result.rows[0].database_zone_config_bytes.length !== 0 ) { zoneConfigData.hexString =
Any way to use the sql function encode(column, 'hex')
to remove the hex logic?
pkg/ui/workspaces/cluster-ui/src/api/tableDetailsApi.ts
line 333 at r1 (raw file):
// Table heuristics details. type TableHeuristicsDetails = TableHeuristicDetailsRow & {
Any reason you aren't using the new SqlApiResponse for all these types?
pkg/ui/workspaces/cluster-ui/src/api/tableDetailsApi.ts
line 420 at r1 (raw file):
if (!(!txn_result.rows || txn_result.rows?.length === 0)) { resp.stats.pebble_data.approximate_disk_bytes = txn_result.rows[0].approximate_disk_bytes;
Is there a guarantee that it will always be 1 row? If it is please add a check and log to console if there is more than 1 row. It's very hard to identify and root cause partial results.
pkg/ui/workspaces/cluster-ui/src/api/tableDetailsApi.ts
line 518 at r1 (raw file):
[new Identifier(dbName)], ), arguments: [escFullTableName.SQLString(), dbName],
Should we get both the name and ids for the dbs/tables to avoid the overhead of the lookup?
pkg/ui/workspaces/cluster-ui/src/api/util.ts
line 44 at r1 (raw file):
} export const stripLeadingHexMarker = (hexString: string): string => {
Should FixFingerprintHexValue be used instead?
24ebd5e
to
ef78264
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @j82w)
pkg/ui/workspaces/cluster-ui/src/api/tableDetailsApi.ts
line 289 at r1 (raw file):
Previously, j82w (Jake) wrote…
nit:
txn_result.rows[0]
seems to be accessed a lot in this method. Why not set it to a const and use that everywhere to avoid the long lines of code?
Done.
pkg/ui/workspaces/cluster-ui/src/api/tableDetailsApi.ts
line 292 at r1 (raw file):
Previously, j82w (Jake) wrote…
Any way to use the sql function
encode(column, 'hex')
to remove the hex logic?
Done, able to remove some but not all.
pkg/ui/workspaces/cluster-ui/src/api/tableDetailsApi.ts
line 333 at r1 (raw file):
Previously, j82w (Jake) wrote…
Any reason you aren't using the new SqlApiResponse for all these types?
Discussed offline, opted to use SqlApiQueryResponse
to capture query-level errors.
pkg/ui/workspaces/cluster-ui/src/api/tableDetailsApi.ts
line 420 at r1 (raw file):
Previously, j82w (Jake) wrote…
Is there a guarantee that it will always be 1 row? If it is please add a check and log to console if there is more than 1 row. It's very hard to identify and root cause partial results.
Done.
pkg/ui/workspaces/cluster-ui/src/api/tableDetailsApi.ts
line 518 at r1 (raw file):
Previously, j82w (Jake) wrote…
Should we get both the name and ids for the dbs/tables to avoid the overhead of the lookup?
I'd like to do this in a follow-up PR that we could backport. Personally, I'd like to get names and ids for databases in the databasesApi.ts
and introduce a similar api for tables tablesListApi.ts
to get table names/ids.
pkg/ui/workspaces/cluster-ui/src/api/util.ts
line 44 at r1 (raw file):
Previously, j82w (Jake) wrote…
Should FixFingerprintHexValue be used instead?
Removed this function, using encode(..., 'hex')
be825eb
to
f771195
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 16 of 16 files at r4, 6 of 6 files at r5, 8 of 8 files at r6, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @j82w)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 16 files at r4.
Reviewable status: complete! 2 of 0 LGTMs obtained (waiting on @THardy98)
pkg/ui/workspaces/cluster-ui/src/api/tableDetailsApi.ts
line 137 at r4 (raw file):
); } if (txn_result.error) {
nit: if is unnecessary
}; | ||
} | ||
|
||
export type TableDetailsResponse = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this type already being used across the pages before this? Otherwise can you change these fields to camelCase to follow convention? If it's being used already (e.g. in tables etc) we can hold off.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed the top level fields to use camelCase. Inner fields follow the same format that's being returned by the sql api which is snake case.
// Table schema details. | ||
type TableSchemaDetails = TableSchemaDetailsRow & { | ||
error?: Error; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: add new lines after these types
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
).length; | ||
return { | ||
name: table, | ||
loading: !!details?.inFlight, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a ticket to change this mapStateFromProps with some selectors on fields to a regular function rather than a selector?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't believe so.
@@ -76,7 +72,6 @@ export const mapStateToProps = createSelector( | |||
hasAdminRole, | |||
): DatabaseTablePageData => { | |||
const details = tableDetails[generateTableID(database, table)]; | |||
const stats = tableStats[generateTableID(database, table)]; | |||
const indexStats = indexUsageStats[generateTableID(database, table)]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same with this mapStateToProps
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto.
This commit migrates our existing admin API table details endpoint to use sql-over-http in cluster-ui. We change the structure of the table details response, allowing each field to be a self-contained query response. The exception here is the `stats` field, which is comprised of a couple query responses. Changing the structure scopes errors at the query-level, instead of the response level. This is important as it allows us to still make use of partial data on the UI when only a subset of the queries succeed. The commit also coalesces the former table stats and table details API into a single table details API. The only difference/movement here is that the table details endpoint now fetches span statistics. The data for table details is populated using 10 queries, which fetch the table's: - ID - grants - schema details (columns & indexes) - create statement - zone configuration statement - zone configuration - heuristics details (specifically, the timestamp at which table heuristics were last updated) - span statistics - index usage statistics - replicas This largely reflects like for like how we fetch data from the existing admin api endpoint. We can improve upon this (i.e. remove bloating) in follow up PRs. Release note: None https://cockroachlabs.atlassian.net/browse/DOC-1355 #Informs: cockroachdb#33316 #Epic: CRDB-8035
Update the database details page to reflect the new response structure. Release note: None https://cockroachlabs.atlassian.net/browse/DOC-1355 #Informs: cockroachdb#33316 #Epic: CRDB-8035
e321bc1
to
aa2327b
Compare
Update the database table page to reflect the new response structure. Release note: None
aa2327b
to
025daac
Compare
bors r+ |
Build failed: |
bors r+ |
Build succeeded: |
Part of: #89429
Resolves: #90264, #90268
This PR migrates our existing admin API table details endpoint to
use sql-over-http in cluster-ui. We change the structure of the table
details response, allowing each field to be a self-contained query
response. The exception here is the
stats
field, which is comprised ofa couple query responses. Changing the structure scopes errors at the
query-level, instead of the response level. This is important as it
allows us to still make use of partial data on the UI when only a subset
of the queries succeed.
The commit also coalesces the former table stats and table details API
into a single table details API. The only difference/movement here is
that the table details endpoint now fetches span statistics.
The data for table details is populated using 10 queries, which fetch
the table's:
heuristics were last updated)
This largely reflects like for like how we fetch data from the existing
admin api endpoint. We can improve upon this (i.e. remove bloating) in
follow up PRs.
Short Demo
https://www.loom.com/share/698c10c5752e439a9edb0ec2301771c1
Release note: None