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: refine hook to schedule data refreshes #97516

Merged
merged 1 commit into from
Mar 23, 2023

Conversation

xinhaoz
Copy link
Member

@xinhaoz xinhaoz commented Feb 22, 2023

This commit refines the logic in the useFetchDataWithPolling hook. Namely, it has been renamed to useScheduleFunction to be more generic. It also fixes a bug where the first call would not occur if shouldPoll was false. This mainly affects the insights pages where if the page was loaded with a custom time interval, the request would not be made.

Epic: none

Release note: None

@xinhaoz xinhaoz requested review from a team February 22, 2023 21:02
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@xinhaoz xinhaoz force-pushed the refine-schedule-func-hook branch 3 times, most recently from 82905b0 to 8b0ca2a Compare February 28, 2023 16:23
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 3 of 4 files at r2, 1 of 1 files at r3.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @xinhaoz)


-- commits line 7 at r3:
isn't the issue that this value should have been true then?


pkg/ui/workspaces/cluster-ui/src/util/hooks.ts line 37 at r3 (raw file):

 * @param callbackFn The call back function to be called at the provided interval.
 * @param scheduleIntervalMs scheduling interval in millis
 * @param shouldReschedule reschedule the function after completion

nit: the phrasing make it sounds like the shouldReschedule is a function that does the reschedule itself. Clarify that this is just a parameter that decided if you should reschedule or not


pkg/ui/workspaces/cluster-ui/src/util/hooks.spec.tsx line 34 at r3 (raw file):

  it("should schedule the function according to the lastCompleted time and interval provided", async () => {
    const timeoutMs = 3000;

can you use smaller duration so the test doesn't take too long?


pkg/ui/workspaces/cluster-ui/src/util/hooks.spec.tsx line 52 at r3 (raw file):

      { timeout: timeoutMs * 3 + 1500 },
    );
    // 3 intervals and the initial call on mount, since last completed was iniitially null.

nit: initially

This commit refines the logic in the `useFetchDataWithPolling`
hook. Namely, it has been renamed to `useScheduleFunction` to
be more generic. It also fixes a bug in the insights pages
where we would not update stale data  if `shouldPoll` was false
(which is the case for custom time intervals). We fix this by
always fetching immediately if the data is invalid.

Tests were also added to verify the hook behaviour.

Epic: none

Release note: None
@xinhaoz xinhaoz force-pushed the refine-schedule-func-hook branch from 8b0ca2a to 8c01d38 Compare March 22, 2023 16:38
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 @maryliag)


-- commits line 7 at r3:

Previously, maryliag (Marylia Gutierrez) wrote…

isn't the issue that this value should have been true then?

We keep it as false because we still do not want to poll when the time picker is on a custom time range. The fix is that we now 'restart' the polling if the data is invalid.


pkg/ui/workspaces/cluster-ui/src/util/hooks.ts line 37 at r3 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

nit: the phrasing make it sounds like the shouldReschedule is a function that does the reschedule itself. Clarify that this is just a parameter that decided if you should reschedule or not

Done.


pkg/ui/workspaces/cluster-ui/src/util/hooks.spec.tsx line 34 at r3 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

can you use smaller duration so the test doesn't take too long?

Done.


pkg/ui/workspaces/cluster-ui/src/util/hooks.spec.tsx line 52 at r3 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

nit: initially

Done.

@xinhaoz xinhaoz requested a review from a team March 22, 2023 19:24
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 4 of 4 files at r4, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @xinhaoz)

@xinhaoz xinhaoz added the backport-23.1.x Flags PRs that need to be backported to 23.1 label Mar 23, 2023
@xinhaoz
Copy link
Member Author

xinhaoz commented Mar 23, 2023

TFTR!
bors r+

@craig
Copy link
Contributor

craig bot commented Mar 23, 2023

Build succeeded:

@craig craig bot merged commit 591dda2 into cockroachdb:master Mar 23, 2023
@xinhaoz xinhaoz deleted the refine-schedule-func-hook branch June 5, 2023 17:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-23.1.x Flags PRs that need to be backported to 23.1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants