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: set index created time as last used time #78283

Merged
merged 1 commit into from
Mar 25, 2022

Conversation

THardy98
Copy link

Resolves: #73766

Previously, database table page on db-console displayed "Never" when
there was no Last Read or Last Reset timestamp. This change sets the
default message to display the creation time of the timestamp if it
exists.

Release note (ui change): add index created time as an option on the
database table page

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@THardy98 THardy98 force-pushed the create_time_default_index branch 2 times, most recently from 7367ee5 to 144eef0 Compare March 22, 2022 19:57
@THardy98 THardy98 requested a review from a team March 23, 2022 13:46
Copy link
Contributor

@matthewtodd matthewtodd left a comment

Choose a reason for hiding this comment

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

Nice! :lgtm:, just one question about the switch statement.

Reviewed 1 of 5 files at r1, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @matthewtodd and @THardy98)


pkg/ui/workspaces/cluster-ui/src/databaseTablePage/databaseTablePage.tsx, line 247 at r1 (raw file):

      return "Never";
    }
    switch (indexStat.lastUsedType) {

Is it okay to get rid of this switch statement now, and just always return formatDate(...)?

Copy link
Author

@THardy98 THardy98 left a comment

Choose a reason for hiding this comment

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

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


pkg/ui/workspaces/cluster-ui/src/databaseTablePage/databaseTablePage.tsx, line 247 at r1 (raw file):

Previously, matthewtodd (Matthew Todd) wrote…

Is it okay to get rid of this switch statement now, and just always return formatDate(...)?

Definitely, I wasn't certain if listing the cases would make it clearer what's happening here or not, but I suppose it's unnecessary.

@THardy98 THardy98 force-pushed the create_time_default_index branch from 144eef0 to 5988855 Compare March 23, 2022 18:20
Copy link
Author

@THardy98 THardy98 left a comment

Choose a reason for hiding this comment

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

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


pkg/ui/workspaces/cluster-ui/src/databaseTablePage/databaseTablePage.tsx, line 247 at r1 (raw file):

Previously, THardy98 (Thomas Hardy) wrote…

Definitely, I wasn't certain if listing the cases would make it clearer what's happening here or not, but I suppose it's unnecessary.

Removed the switch statement.

Copy link
Contributor

@maryliag maryliag left a comment

Choose a reason for hiding this comment

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

Reviewed 5 of 5 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @matthewtodd and @THardy98)


pkg/ui/workspaces/db-console/src/views/databases/databaseTablePage/redux.spec.ts, line 146 at r2 (raw file):

}

describe.only("Database Table Page", function() {

nit: remove the only before merging

Copy link
Contributor

@matthewtodd matthewtodd left a comment

Choose a reason for hiding this comment

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

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


pkg/ui/workspaces/db-console/src/views/databases/databaseTablePage/redux.spec.ts, line 146 at r2 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

nit: remove the only before merging

Good catch!

Previously, database table page on db-console displayed "Never" when there was
no Last Read or Last Reset timestamp. This change sets the default message to
display the creation time of the timestamp if it exists.

Release note (ui change): add index created time as an option on the db-console
database table page
@THardy98 THardy98 force-pushed the create_time_default_index branch from 5988855 to 761837d Compare March 24, 2022 18:59
Copy link
Author

@THardy98 THardy98 left a comment

Choose a reason for hiding this comment

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

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


pkg/ui/workspaces/db-console/src/views/databases/databaseTablePage/redux.spec.ts, line 146 at r2 (raw file):

Previously, matthewtodd (Matthew Todd) wrote…

Good catch!

^ that goes double :)

@THardy98 THardy98 requested a review from maryliag March 24, 2022 20:44
Copy link
Contributor

@maryliag maryliag left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (and 1 stale) (waiting on @matthewtodd)

@THardy98
Copy link
Author

bors r+

1 similar comment
@THardy98
Copy link
Author

bors r+

@craig
Copy link
Contributor

craig bot commented Mar 25, 2022

Build succeeded:

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.

Add Create time as the default Last Used time for Index Stats table
4 participants