-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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/cluster-ui: fix polling in fingerprints pages #85772
Conversation
c72fa5d
to
719e062
Compare
87b4467
to
e4aa72e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @maryliag and @xinhaoz)
-- commits
line 11 at r1:
nit: request
when the time selected is "last hour", "last month" and so on, we still want to keep refreshing, we just don't refresh if is a custom value
we need
pkg/ui/workspaces/cluster-ui/src/store/sqlStats/sqlStats.sagas.ts
line 80 at r1 (raw file):
try { yield call(resetSQLStats); yield put(sqlStatsActions.invalidated());
why is this no longer required?
1c36edc
to
1d85377
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @maryliag and @xinhaoz)
Previously, maryliag (Marylia Gutierrez) wrote…
nit: request
when the time selected is "last hour", "last month" and so on, we still want to keep refreshing, we just don't refresh if is a custom value
we need
Changed my approach to handle this!
pkg/ui/workspaces/cluster-ui/src/store/sqlStats/sqlStats.sagas.ts
line 65 at r2 (raw file):
export function* resetSQLStatsSaga(action: PayloadAction<StatementsRequest>) { try { yield call(resetSQLStats);
At a point in the past, a call toinvalidated
would refresh the stats (and may have been a requirement in order for the request to go through). This is no longer necessary, and as of this PR, invalidated
is called directly in the request action to signify that new data is being fetched.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 5 files at r1, 9 of 11 files at r2, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @maryliag and @xinhaoz)
pkg/ui/workspaces/cluster-ui/src/store/sqlStats/sqlStats.sagas.ts
line 35 at r2 (raw file):
action: PayloadAction<StatementsRequest>, ): any { yield put(sqlStatsActions.invalidated());
is you invalidate before each request, doesn't this means that if you have a scale as "last 1h", when you're about to refresh, the info on the page will disappear, since is no longer valid? having the same problem of blank page being refreshed?
1d85377
to
5592c49
Compare
5592c49
to
4f86ce1
Compare
Previously, maryliag (Marylia Gutierrez) wrote…
Yes the invalidation is necessary to show the loading spinner. I took another look and my reasoning for the bug was wrong, so I've updated the PR. I initially thought Now that the invalidation is moved right before we make the request, we shouldn't have problems with that anymore. Also removing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a comment to change the fixes to partially addresses, otherwise
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @maryliag and @xinhaoz)
pkg/ui/workspaces/cluster-ui/src/store/sqlStats/sqlStats.sagas.ts
line 35 at r2 (raw file):
Previously, xinhaoz (Xin Hao Zhang) wrote…
Yes the invalidation is necessary to show the loading spinner. I took another look and my reasoning for the bug was wrong, so I've updated the PR. I initially thought
throttleWithReset
was responsible for the bug and was making the api calls. I then remembered we previously had the refresh inshouldComponentUpdate
, and sothrottleWithReset
was just for preventing those calls from making a request before the delay was up. The real issue was the invalidation in the receivedSqlStats saga, which fired after the cache invalidation. This was likely to trigger the loading screen, or maybe leftover behaviour from a time when we only fetched data if it was invalid (this is what cachedDataReducer does). So since we were invalidating the data without ever refreshing it before, the pages would get stuck on loading. I think one improvement here (but out of scope for this PR) would be to introduce a new inFlight param to the state to better separate data validity and when a request is in flight (this also exists in the db-console side already).Now that the invalidation is moved right before we make the request, we shouldn't have problems with that anymore. Also removing
throttleWIthReset
still makes sense since we use the timeout in the pages directly to manage the polling interval now.
I assume the scope of the PR was to fix the problem of "showing a loading instead of the actual data on the page". Seems like this PR fixes the part about the data actually being refreshed, and we need another PR to continue showing the data even during a refresh (which is specially important on the cases we have a lot of data and it takes minutes to load)
In that case, change the description to partially addresses the issue, and once you have the second PR, then it can be considered fixed
Previously, maryliag (Marylia Gutierrez) wrote…
Oh we can introduce that behaviour quite easily. I always thought we wanted to show the loading spinner when a new request was pending though -- which specific cases do we not want to show that? Is it just when the source of the request is from polling rather than a change in time range? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @maryliag and @xinhaoz)
pkg/ui/workspaces/cluster-ui/src/store/sqlStats/sqlStats.sagas.ts
line 35 at r2 (raw file):
Previously, xinhaoz (Xin Hao Zhang) wrote…
Oh we can introduce that behaviour quite easily. I always thought we wanted to show the loading spinner when a new request was pending though -- which specific cases do we not want to show that? Is it just when the source of the request is from polling rather than a change in time range?
That's correct. Basically, if there was some user interaction (change date, refresh page), we can show the loading. If it's just us updating the data, we don't want to distract the user, so that case shouldn't display any loading
300fe1e
to
99fc418
Compare
Updated to match behaviour above, so requesting a re-review! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, thank you for updating it!
Reviewed 1 of 11 files at r2, 9 of 9 files at r4, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @maryliag)
Fixes: cockroachdb#85236 Previously, SQL statement and transaction stats would be refreshed every 5 minutes via sagas in CC and the cached api reducer in db-console. This method relied on a refresh data call that resided in `shouldComponentUpdate`, which was ignored by the respective polling managers when the time interval was not complete. This pattern was hacky as (unguarded calls in `shouldComponentUpdate` are typically avoided in React. Polling in these pages were removed with the introduciton of persisted stats, however we would like to reintroduce polling when the selected time interval is `Latest xx..' (i.e. not a custom interval). The removal of this polling introduced a bug in the CC fingerprint pages, as the saga effects for when the data was received would invalidate the data after the polling interval. Now that the data was never refreshed, the page would get stuck on the 'loading data' page. This commit reintroduces polling via a `setTimeout` in the page components, rather than through cached data reducer and sagasn for CC. Data in the fingerprints overview pages is now refreshed every 5 minutes for non-custom time ranges. The data invalidation in CC is also cleaned up such that we don't invalidate data after a delay in the receive effect. When data is refreshed via polling, we do not show the loading spinner. Release note (bug fix): the statements and transaction fingerprint will no longer get stuck on the loading page in CC after 5 minutes idling on the page Release note (ui change): the statements and transaction fingerprint now refresh data every 5 minutes for non-custom time ranges Release justification: bug fix
99fc418
to
bed986b
Compare
CI failure doesn't look related so I'm merging, TFTR! |
Build succeeded: |
@xinhaoz can you backport this to 22.1 ? |
This commit moves polling to the statement and transaction details page components from the cached data reducer and sagas, using the same approach as cockroachdb#85772. Fixes cockroachdb#91297. Release note (ui change): The statement fingerprint details page in the Console no longer infinitely loads after 5 minutes.
92596: ui: fix polling for statement and transaction details pages r=ericharmeling a=ericharmeling This commit moves polling to the statement and transaction details page components from the cached data reducer and sagas, using the same approach as #85772. Fixes #91297. Loom (DB Console and CC Console): https://www.loom.com/share/d3002ad54463448aaa3b8c9d74e31d10 Release note (ui change): The statement fingerprint details page in the Console no longer infinitely loads after 5 minutes. Co-authored-by: Eric Harmeling <[email protected]>
This commit moves polling to the statement and transaction details page components from the cached data reducer and sagas, using the same approach as cockroachdb#85772. Fixes cockroachdb#91297. Release note (ui change): The statement fingerprint details page in the Console no longer infinitely loads after 5 minutes.
Fixes: #85236
Previously, SQL statement and transaction stats would be refreshed
every 5 minutes via sagas in CC and the cached api reducer in
db-console. This method relied on a refresh data call that resided
in
shouldComponentUpdate
, which was ignored by the respectivepolling managers when the time interval was not complete. This
pattern was hacky as (unguarded calls in
shouldComponentUpdate
are typically avoided in React. Polling in these pages were removed
with the introduciton of persisted stats, however we would like to
reintroduce polling when the selected time interval is `Latest xx..'
(i.e. not a custom interval). The removal of this polling introduced
a bug in the CC fingerprint pages, as the saga effects for when the
data was received would invalidate the data after the polling interval.
Now that the data was never refreshed, the page would get stuck on
the 'loading data' page.
This commit reintroduces polling via a
setTimeout
in the pagecomponents, rather than through cached data reducer and sagasn for CC.
Data in the fingerprints overview pages is now refreshed every
5 minutes for non-custom time ranges. The data invalidation in
CC is also cleaned up such that a call to invalidate data only
happens right before a request to fetch data (to signify new data
is being loaded).
Release note (bug fix): the statements and transaction fingerprint
will no longer get stuck on the loading page in CC after 5 minutes
idling on the page
Release note (ui change): the statements and transaction fingerprint
now refresh data every 5 minutes for non-custom time ranges
Release justification: bug fix