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: fix sql stats page data refreshing #95397

Merged
merged 1 commit into from
Jan 18, 2023
Merged

Conversation

xinhaoz
Copy link
Member

@xinhaoz xinhaoz commented Jan 17, 2023

This commit fixes a couple of bugs with sql stats page data fetching.

First - we missed clearing the initial timeout to fetch data that was created on mount - it should be cleared when a new time range is selected on the page or when the page is unmounted. Currently, this bug can lead to unexpeted data refetches of the incorrect (previously selected) time range.

Second - with the introduction of new pages that use the time picker, such as insights, sql stats pages are no longer the only page that can change the time range. Previously, data was only fetched on mount for sql stats pages if it was not previously fetched, or a data refresh was scheduled based on last update time if the current time range was not a custom (fixed) range. We should now also refetch when the data is outdated due to a new time range being selected on a different page. This commit introduces the isDataValid prop to sql stats pages to determine this and refetch data accordingly.

Epic: none

CC - just showing all the fetching is still working:
https://www.loom.com/share/1b24ec65dbe64dcd9c23cd2be980b877

DB Console -
https://www.loom.com/share/0afe1722d7ab418bb7f41c2462f4d84a

@xinhaoz xinhaoz requested review from a team January 17, 2023 21:32
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Contributor

@ericharmeling ericharmeling left a comment

Choose a reason for hiding this comment

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

Great catch! I noticed just a few things, otherwise LGTM.

Reviewed 12 of 12 files at r1, all commit messages.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @xinhaoz)


pkg/ui/workspaces/cluster-ui/src/statementsPage/statementsPage.tsx line 157 at r1 (raw file):

  RouteComponentProps<unknown>;

function statementsRequestFromProps(

Should we rename this now?

Suggestion:

statementsRequestFromTimeScale

pkg/ui/workspaces/cluster-ui/src/transactionsPage/transactionsPage.tsx line 121 at r1 (raw file):

  RouteComponentProps;

function statementsRequestFromProps(

Suggestion:

statementsRequestFromTimeScale

pkg/ui/workspaces/cluster-ui/src/transactionsPage/transactionsPage.selectors.ts line 23 at r1 (raw file):

);

export const selectTxnsDataValid = createSelector(

This looks like the same selector as selectStatementsDataValid. Could you just use that one?

Code quote:

selectTxnsDataValid 

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 (waiting on @xinhaoz)


pkg/ui/workspaces/cluster-ui/src/statementsPage/statementsPage.tsx line 347 at r1 (raw file):

      nextRefresh = now;
    } else if (this.props.timeScale.key !== "Custom") {
      nextRefresh = this.props.lastUpdated

Same q here about lastUpdated as below.


pkg/ui/workspaces/cluster-ui/src/transactionsPage/transactionsPage.tsx line 241 at r1 (raw file):

      nextRefresh = now;
    } else if (this.props.timeScale.key !== "Custom") {
      nextRefresh = this.props.lastUpdated

Can we count on lastUpdated to exist here now? The previous code had this.props.lastUpdated?.clone()..., with the ?.

Copy link
Member Author

@xinhaoz xinhaoz 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 (waiting on @ericharmeling and @matthewtodd)


pkg/ui/workspaces/cluster-ui/src/statementsPage/statementsPage.tsx line 157 at r1 (raw file):

Previously, ericharmeling (Eric Harmeling) wrote…

Should we rename this now?

Done.


pkg/ui/workspaces/cluster-ui/src/transactionsPage/transactionsPage.tsx line 241 at r1 (raw file):

Previously, matthewtodd (Matthew Todd) wrote…

Can we count on lastUpdated to exist here now? The previous code had this.props.lastUpdated?.clone()..., with the ?.

Yup, if lastUpdated is nullish, it would have gone into the previous block.


pkg/ui/workspaces/cluster-ui/src/transactionsPage/transactionsPage.selectors.ts line 23 at r1 (raw file):

Previously, ericharmeling (Eric Harmeling) wrote…

This looks like the same selector as selectStatementsDataValid. Could you just use that one?

Done.

@xinhaoz xinhaoz requested a review from matthewtodd January 18, 2023 14:29
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 (waiting on @ericharmeling and @xinhaoz)


pkg/ui/workspaces/cluster-ui/src/transactionsPage/transactionsPage.tsx line 241 at r1 (raw file):

Previously, xinhaoz (Xin Hao Zhang) wrote…

Yup, if lastUpdated is nullish, it would have gone into the previous block.

🤦 of course. Thanks.

This commit fixes a couple of bugs with sql stats page
data fetching.

First - we missed clearing the initial timeout to fetch data that was
created on mount - it should be cleared when a new time range is
selected on the page or when the page is unmounted. Currently, this
bug can lead to unexpeted data refetches of the incorrect (previously
selected) time range.

Second - with the introduction of new pages that use the time
picker, such as insights, sql stats pages are no longer the only
page that can change the time range. Previously, data was only
fetched on mount for sql stats pages if it was not previously fetched,
or a data refresh was scheduled based on last update time if the
current time range was not a custom (fixed) range. We should now also
refetch when the data is outdated due to a new time range being
selected on a different page. This commit introduces the `isDataValid`
prop to sql stats pages to determine this and refetch data accordingly.

Epic: none
@xinhaoz
Copy link
Member Author

xinhaoz commented Jan 18, 2023

TFTR!
bors r+

@craig
Copy link
Contributor

craig bot commented Jan 18, 2023

Build succeeded:

@craig craig bot merged commit 1d2ba22 into cockroachdb:master Jan 18, 2023
@blathers-crl
Copy link

blathers-crl bot commented Jan 18, 2023

Encountered an error creating backports. Some common things that can go wrong:

  1. The backport branch might have already existed.
  2. There was a merge conflict.
  3. The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.


error creating merge commit from 9247472 to blathers/backport-release-22.2-95397: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 22.2.x failed. See errors above.


🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.

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.

4 participants