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

cluster-ui: add getTableMetadataApi and connect tables page #131123

Merged
merged 1 commit into from
Sep 23, 2024

Conversation

xinhaoz
Copy link
Member

@xinhaoz xinhaoz commented Sep 20, 2024

Only the latest commit should be reviewed in this PR.
Previous: #131046


This commit connects the v2 db details - tables page to the
new api/v2/table_metadata api route.

The following components are now connected to the api:

  • listing tables for a db
  • sorting by column
  • searching by table name
  • filtering by node id

Epic: CRDB-37558
Fixes: #131122

Release note: None

@xinhaoz xinhaoz requested a review from a team as a code owner September 20, 2024 19:37
@xinhaoz xinhaoz requested review from angles-n-daemons, dhartunian and kyle-a-wong and removed request for a team September 20, 2024 19:37
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@xinhaoz xinhaoz force-pushed the connect-tables-v2 branch 2 times, most recently from 8183448 to 528111f Compare September 20, 2024 21:10
@xinhaoz
Copy link
Member Author

xinhaoz commented Sep 20, 2024

https://www.loom.com/share/9d9ab55daa484920851e8377d5bcca0c

Note that the percentage live data column has a bug in that video that is fixed in #131124

Copy link
Contributor

@kyle-a-wong kyle-a-wong left a comment

Choose a reason for hiding this comment

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

lgtm. Just have a couple small nits

percent_live_data: number;
total_live_data_bytes: number;
total_data_bytes: number;
store_ids: number[] | null;
Copy link
Contributor

Choose a reason for hiding this comment

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

is this nullable / Are you seeing cases where this is null instead of an empty array?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah I just saw that the system schema defines this column as not nullable. Removed.

dbID?: number;
sortBy?: string;
sortOrder?: "asc" | "desc";
storeID?: StoreID[];
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: should this be storeIds? Also same comment about the the capital D

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

type TableMetadataResponse = APIV2ResponseWithPaginationState<TableMetadata[]>;

export type TableMetadataRequest = {
dbID?: number;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I know this is pretty inconsistent in the repo, but I think I personally don't think the "D" in "ID" should be uppercase

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

<div>
{Bytes(t.liveDataBytes)} / {Bytes(t.totalDataBytes)}
<div>{t.liveDataPercentage * 100}%</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: We should maybe talk to Inna about what level of precision we want and should round to that. Also one thing about rounding logic that we should keep in mind is that we shouldnt round "99.99x" up to 100%

Copy link
Member Author

Choose a reason for hiding this comment

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

Ack - yeah let's get her to review and leave design requests.

This commit connects the v2 db details - tables page to the
new `api/v2/table_metadata` api route.

The following components are now connected to the api:
- listing tables for a db
- sorting by column
- searching by table name
- filtering by node id

Epic: CRDB-37558
Fixes: cockroachdb#131122

Release note: None
@xinhaoz
Copy link
Member Author

xinhaoz commented Sep 23, 2024

TFTR!
bors r+

@craig craig bot merged commit 8c1eedf into cockroachdb:master Sep 23, 2024
22 of 23 checks passed
@xinhaoz xinhaoz deleted the connect-tables-v2 branch October 3, 2024 14:43
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.

cluster-ui: connect v2 tables page to table_metadata api
3 participants