ui: unify cluster-ui stores for Statement and Transaction Page#72627
ui: unify cluster-ui stores for Statement and Transaction Page#72627craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
|
This change is |
279f030 to
ab15b8e
Compare
maryliag
left a comment
There was a problem hiding this comment.
Reviewed 19 of 23 files at r1, all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @Azhng and @maryliag)
pkg/ui/workspaces/cluster-ui/src/store/sagas.ts, line 31 at r1 (raw file):
fork(notifificationsSaga), fork(sqlStatsSaga), fork(terminateSaga),
what is this one for?
pkg/ui/workspaces/cluster-ui/src/store/sqlStats/sqlStats.sagas.spec.ts, line 54 at r1 (raw file):
}); it("requests combined SQL Stats if combined=true in the request message", () => {
nit: test also with combined false
pkg/ui/workspaces/db-console/src/redux/sqlStats/sqlStatsSagas.ts, line 33 at r1 (raw file):
yield put(resetSQLStatsCompleteAction()); yield put(invalidateStatements()); yield put(refreshStatements() as any);
we don't want to refresh sql stats here?
ab15b8e to
d5f9d60
Compare
Azhng
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/ui/workspaces/cluster-ui/src/store/sagas.ts, line 31 at r1 (raw file):
Previously, maryliag (Marylia Gutierrez) wrote…
what is this one for?
Removed, I noticed we had an unused import for terminateSaga and thought we just missed it. I just remembered that we disabled that button already.
pkg/ui/workspaces/cluster-ui/src/store/sqlStats/sqlStats.sagas.spec.ts, line 54 at r1 (raw file):
Previously, maryliag (Marylia Gutierrez) wrote…
nit: test also with combined false
Done.
pkg/ui/workspaces/db-console/src/redux/sqlStats/sqlStatsSagas.ts, line 33 at r1 (raw file):
Previously, maryliag (Marylia Gutierrez) wrote…
we don't want to refresh sql stats here?
I am a bit uncertain about this. If we invalidate the state, this would cause the selector to return null, which in turn will trigger the componentDidUpdate() React component hook, which in turn will invoke the refreshStatement sagas anyway.
That's why I'm a bit not so sure if we should let the React component to handle this or if we should let the Sagas to handle this. Thoughts ?
maryliag
left a comment
There was a problem hiding this comment.
Reviewed 1 of 23 files at r1, 4 of 4 files at r2, all commit messages.
Reviewable status:
pkg/ui/workspaces/db-console/src/redux/sqlStats/sqlStatsSagas.ts, line 33 at r1 (raw file):
Previously, Azhng (Archer Zhang) wrote…
I am a bit uncertain about this. If we invalidate the state, this would cause the selector to return
null, which in turn will trigger thecomponentDidUpdate()React component hook, which in turn will invoke therefreshStatementsagas anyway.That's why I'm a bit not so sure if we should let the React component to handle this or if we should let the Sagas to handle this. Thoughts ?
If we keep the refresh here, is the refreshStatement being called twice? (here and on the component did mount), or if we call it here it doesn't get called there again?
If is being called twice, there isn't need for the refresh here, otherwise we can keep this here
Previously, on CC Conosle, Statement Page and Transaction Page were backed by their own Redux stores and their own Redux Sagas. However, both Statement and Transaction Page were backed by a single API endpoint, namely `/_status/statements`. This resulted in unnecessary API calls and bugs like cockroachdb#72009, which was caused by Reset SQL Stats sagas failing to reset the Transaction Page Redux store. This commit unifies the following Redux store and sagas into a single store / sagas: 1. Statement Page store / sagas 2. Transaciton Page store / sagas 3. Reset SQL Stats store / sagas This greatly removed the code duplication and test duplication and simplify the logic. Statement Page and Transaction Page can reuse the same store by their own selectors. Resolves cockroachdb#72009 Release note: None
d5f9d60 to
f35c4a8
Compare
Azhng
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/ui/workspaces/db-console/src/redux/sqlStats/sqlStatsSagas.ts, line 33 at r1 (raw file):
Previously, maryliag (Marylia Gutierrez) wrote…
If we keep the refresh here, is the refreshStatement being called twice? (here and on the component did mount), or if we call it here it doesn't get called there again?
If is being called twice, there isn't need for the refresh here, otherwise we can keep this here
Hmm thinking about this a bit more, if we refresh here, then yes refreshStatement() will called twice, (once here and once on the component did update).
However I think that is probably fine, since refreshStatement() is only issuing refresh redux actions. Since this is backed by a CachedDataReducer, two subsequent refresh actions will only result in API calls.
I think letting the saga to deal with the refresh would be ideal for the long term, as we move to use functional components and no longer rely on React lifecycle method in the future.
Added refreshStatements() call here.
maryliag
left a comment
There was a problem hiding this comment.
Reviewed 4 of 4 files at r3, all commit messages.
Reviewable status:
|
TFTR! bors r=maryliag |
|
Build failed (retrying...): |
|
Build failed (retrying...): |
|
Build failed: |
|
bors r=maryliag |
|
Build succeeded: |
Previously, on CC Conosle, Statement Page and Transaction Page were backed by
their own Redux stores and their own Redux Sagas. However, both Statement
and Transaction Page were backed by a single API endpoint, namely
/_status/statements. This resulted in unnecessary API calls and bugslike #72009, which was caused by Reset SQL Stats sagas failing to
reset the Transaction Page Redux store.
This commit unifies the following Redux store and sagas into a single
store / sagas:
This greatly removed the code duplication and test duplication and simplify
the logic. Statement Page and Transaction Page can reuse the same store
by their own selectors.
Resolves #72009
Release note: None
CC Console:
cc.mov
DB Console:
Screen.Recording.2021-11-09.at.23.07.17.mov