ui: show regions in SQL Activity for tenants #92357
Conversation
|
This change is |
|
Bazel Extended CI failures are unrelated, being fixed in #93616. |
There was a problem hiding this comment.
Reviewed 10 of 10 files at r1, all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @matthewtodd)
pkg/ui/workspaces/cluster-ui/src/statementDetails/statementDetailsConnected.ts line 65 at r1 (raw file):
statementsError: lastError, timeScale: selectTimeScale(state), // TODO(todd): Remove this unused property!
Is there a reason this can't be done in this PR?
pkg/ui/workspaces/cluster-ui/src/statementsTable/statementsTable.tsx line 307 at r1 (raw file):
className: cx("statements-table__col-regions"), cell: (stmt: AggregateStatistics) => { return longListWithTooltip(stmt.regions.sort().join(", "), 50);
Would it be better to add a field to the stmt object and doing the transformation when getting the data to avoid the duplicate code? It would also avoid needing to compute the string twice.
There was a problem hiding this comment.
Reviewable status:
pkg/ui/workspaces/cluster-ui/src/statementDetails/statementDetailsConnected.ts line 65 at r1 (raw file):
Previously, j82w wrote…
Is there a reason this can't be done in this PR?
When I was here I was pretty sure it was completely unused, but I wanted to look further, and it's unrelated enough to this PR that I wanted to split it out.
pkg/ui/workspaces/cluster-ui/src/statementsTable/statementsTable.tsx line 307 at r1 (raw file):
Previously, j82w wrote…
Would it be better to add a field to the stmt object and doing the transformation when getting the data to avoid the duplicate code? It would also avoid needing to compute the string twice.
Yeah, I tried that first but ran into complications trying to make the rest of the moving parts cooperate.
There was a problem hiding this comment.
Reviewed 10 of 10 files at r1.
Reviewable status:
pkg/ui/workspaces/cluster-ui/src/transactionsPage/utils.ts line 261 at r1 (raw file):
nodeRegions: { [p: string]: string }, ): string[] => { const regions: { [region: string]: Set<number> } = {};
If the function returns only the keys of regions (a list of strings), then would it make more sense to make regions a set instead of a map?
There was a problem hiding this comment.
Reviewable status:
pkg/ui/workspaces/cluster-ui/src/statementDetails/statementDetailsConnected.ts line 65 at r1 (raw file):
Previously, matthewtodd (Matthew Todd) wrote…
When I was here I was pretty sure it was completely unused, but I wanted to look further, and it's unrelated enough to this PR that I wanted to split it out.
WIP branch here, will send as a PR once this one lands.
There was a problem hiding this comment.
Reviewable status:
pkg/ui/workspaces/cluster-ui/src/transactionsPage/utils.ts line 261 at r1 (raw file):
Previously, gtr (Gerardo Torres Castro) wrote…
If the function returns only the keys of
regions(a list of strings), then would it make more sense to makeregionsa set instead of a map?
Yes, that makes sense. I copied this from some nearby code without thinking too much. Let me tidy it up real quick...
Part of #89949. Release note (ui change): The console statement and transaction pages for tenant clusters gained region columns and filters for multiregion tenants.
There was a problem hiding this comment.
Reviewable status:
There was a problem hiding this comment.
Reviewable status:
pkg/ui/workspaces/cluster-ui/src/transactionsPage/utils.ts line 261 at r1 (raw file):
Previously, matthewtodd (Matthew Todd) wrote…
Yes, that makes sense. I copied this from some nearby code without thinking too much. Let me tidy it up real quick...
Tidied!
There was a problem hiding this comment.
Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status:
|
Thanks, guys! bors r+ |
|
Build succeeded: |
Fixes #89949.
Statements
Transactions
Release note (ui change): The console statement and transaction pages for tenant clusters gained region columns and filters for multiregion tenants.