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

release-22.2: ui: fix sql stats page data refreshing #95788

Merged
merged 1 commit into from
Jan 25, 2023

Conversation

xinhaoz
Copy link
Member

@xinhaoz xinhaoz commented Jan 24, 2023

Backport 1/1 commits from #95397.

/cc @cockroachdb/release


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

Release justification: bug fix

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
@blathers-crl
Copy link

blathers-crl bot commented Jan 24, 2023

Thanks for opening a backport.

Please check the backport criteria before merging:

  • Patches should only be created for serious issues or test-only changes.
  • Patches should not break backwards-compatibility.
  • Patches should change as little code as possible.
  • Patches should not change on-disk formats or node communication protocols.
  • Patches should not add new functionality.
  • Patches must not add, edit, or otherwise modify cluster versions; or add version gates.
If some of the basic criteria cannot be satisfied, ensure that the exceptional criteria are satisfied within.
  • There is a high priority need for the functionality that cannot wait until the next release and is difficult to address in another way.
  • The new functionality is additive-only and only runs for clusters which have specifically “opted in” to it (e.g. by a cluster setting).
  • New code is protected by a conditional check that is trivial to verify and ensures that it only runs for opt-in clusters.
  • The PM and TL on the team that owns the changed code have signed off that the change obeys the above rules.

Add a brief release justification to the body of your PR to justify this backport.

Some other things to consider:

  • What did we do to ensure that a user that doesn’t know & care about this backport, has no idea that it happened?
  • Will this work in a cluster of mixed patch versions? Did we test that?
  • If a user upgrades a patch version, uses this feature, and then downgrades, what happens?

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@xinhaoz xinhaoz requested a review from a team January 24, 2023 20:14
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:

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained

@xinhaoz xinhaoz merged commit 9de5728 into cockroachdb:release-22.2 Jan 25, 2023
@xinhaoz xinhaoz deleted the backport22.2-95397 branch January 25, 2023 18:54
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.

3 participants