ui: Add page to view schedules in DB console. #86409
Conversation
|
This change is |
|
Hi Ben, can you provide a few screenshots in the description? Just to help product folks (PM, Docs, Design etc.) know the scope of the change and provide feedback where we can :) Example issue which does this |
|
🤦 Absolutely! I took the screenshots, just forgot to attach them :) Thanks @kevin-v-ngo ! |
There was a problem hiding this comment.
Hi Ben, the pages look great! I do have a few suggestions 😄
When you're making big changes on the UI, it's a good practice to also add a video of the page working, clicking things around, selecting different values on the dropdown, etc. We do have access to enterprise account on loom, so you can use that https://www.loom.com/looms/videos
If you plan to ever have this page available on cloud, I wouldn't use the admin.go endpoint, because is not available there. Also we do have a new SQL over http API that we're leaning to use from now on, avoiding the need to create several endpoint, and now you can use directly on the UI. Here is an example of how we're using:
Also, I'm not sure what I should be looking on the first screenshot, it doesn't look like that page has any changes
Reviewed 37 of 39 files at r1.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @benbardin and @maryliag)
-- commits line 2 at r1:
nit: use all lower case for the title
can you add more explanation about what this PR is about, what info expect to see, which issues is related to, etc?
pkg/ui/workspaces/cluster-ui/src/schedules/scheduleDetailsPage/scheduleDetailsConnected.tsx line 2 at r1 (raw file):
// Copyright 2022 The Cockroach Authors. //
this file would only be used if the page exists on CC Console. Is the plan to add on CC Console. If so, as my other comment mentioned, it won't work if your endpoint is on admin.go
I assume you used the Jobs page as your guide, but we did add that to Cloud and made changes on the specific admin endpoint for jobs to allow that. So make sure you're testing if this works on Cloud.
pkg/ui/workspaces/cluster-ui/src/schedules/schedulesPage/schedulesPage.fixture.tsx line 259 at r1 (raw file):
const history = createMemoryHistory({ initialEntries: ["/statements"] }); const staticScheduleProps: Pick<
nit: since you list for pick is big, you can use Omit<... instead, and just add the 4 you don't care about:
schedules
schedulesError
schedulesLoading
onFilterChange
pkg/ui/workspaces/cluster-ui/src/schedules/schedulesPage/schedulesPageConnected.tsx line 3 at r1 (raw file):
// Copyright 2022 The Cockroach Authors. // // Use of this software is governed by the Business Source License
same comment as the other "Connected" file. This is only used if you have the page on CC Console
pkg/ui/workspaces/cluster-ui/src/schedules/schedulesPage/scheduleTable.tsx line 127 at r1 (raw file):
</Tooltip> ), cell: schedule => schedule.recurrence,
Not sure if that is the design decision, but having @hourly @weekly looks a little weird, I would prefer if you can so schedule => schedule.recurrence.substring(1) to remove the @ and have just the text
Just my opinion, feel free to ignore if that is by design
pkg/ui/workspaces/cluster-ui/src/store/scheduleDetails/index.ts line 12 at r1 (raw file):
export * from "./schedule.reducer"; export * from "./schedule.sagas";
these 2 files (reducer, sagas) are only used for CC Console
pkg/ui/workspaces/cluster-ui/src/store/schedules/index.ts line 13 at r1 (raw file):
export * from "./schedules.reducer"; export * from "./schedules.sagas"; export * from "./schedules.selectors";
same thing as the other comments, reducer and saga is only used on CC Console
benbardin
force-pushed
the
asui2
branch
4 times, most recently
from
92b6ba5
to
a456a0a
Compare
|
It's great that we're providing more observability for schedules! I'm adding @jeremycbloom, @livlobo, and @thtruo. Wasn't sure if there were designs for this. I remember wanting to avoid or at least having guidelines on the top level information architecture when introducing top-level left nav to avoid info overload - Schedules also seems very related to Jobs. Tommy, we'll need to update the CockroachUX ownership here. |
There was a problem hiding this comment.
Added a loom video, and switched to the new SQL endpoint! The first screenshot is meant to show the Schedules option in the sidebar, but that's a heck of a lot clearer in the video!
Reviewable status:
Previously, maryliag (Marylia Gutierrez) wrote…
nit: use all lower case for the title
can you add more explanation about what this PR is about, what info expect to see, which issues is related to, etc?
done!
pkg/ui/workspaces/cluster-ui/src/schedules/scheduleDetailsPage/scheduleDetailsConnected.tsx line 2 at r1 (raw file):
Previously, maryliag (Marylia Gutierrez) wrote…
this file would only be used if the page exists on CC Console. Is the plan to add on CC Console. If so, as my other comment mentioned, it won't work if your endpoint is on admin.go
I assume you used the Jobs page as your guide, but we did add that to Cloud and made changes on the specific admin endpoint for jobs to allow that. So make sure you're testing if this works on Cloud.
Moved to the generic SQL endpoint - I plan to migrate this to cloud in a later diff.
pkg/ui/workspaces/cluster-ui/src/schedules/schedulesPage/schedulesPage.fixture.tsx line 259 at r1 (raw file):
schedules
schedulesError
schedulesLoading
onFilterChange
done
pkg/ui/workspaces/cluster-ui/src/schedules/schedulesPage/schedulesPageConnected.tsx line 3 at r1 (raw file):
Previously, maryliag (Marylia Gutierrez) wrote…
same comment as the other "Connected" file. This is only used if you have the page on CC Console
removed
pkg/ui/workspaces/cluster-ui/src/schedules/schedulesPage/scheduleTable.tsx line 127 at r1 (raw file):
Previously, maryliag (Marylia Gutierrez) wrote…
Not sure if that is the design decision, but having
@hourly@weeklylooks a little weird, I would prefer if you can soschedule => schedule.recurrence.substring(1)to remove the@and have just the text
Just my opinion, feel free to ignore if that is by design
Yeah, it's intentional - This column is actually a cron expression, I just don't like typing things like "3 4 5 * *" in my demos :) "@hourly" is a (semi-)standard cron syntax here, so it's probably most reasonable to keep it consistent.
pkg/ui/workspaces/cluster-ui/src/store/scheduleDetails/index.ts line 12 at r1 (raw file):
Previously, maryliag (Marylia Gutierrez) wrote…
these 2 files (reducer, sagas) are only used for CC Console
removed
pkg/ui/workspaces/cluster-ui/src/store/schedules/index.ts line 13 at r1 (raw file):
Previously, maryliag (Marylia Gutierrez) wrote…
same thing as the other comments, reducer and saga is only used on CC Console
removed
There was a problem hiding this comment.
Yes, they are very related! I'd certainly be open to consolidating Schedules and Jobs into a single tab. That said, given the approaching branch cut I do think there's value in shipping what we have - that way we can start getting feedback, and this is easy enough to revise for a later release. But if others notice pitfalls I've missed I'm glad to hear them!
All that said, tests are now 💚 so I think this is ready for another review
Reviewable status:
There was a problem hiding this comment.
Thank you for working on the changes! It looks great!! I have just one more suggestion about how you're getting the details :)
Reviewed 14 of 36 files at r2, 2 of 3 files at r3, all commit messages.
Reviewable status:
pkg/ui/workspaces/cluster-ui/src/api/schedulesApi.ts line 23 at r3 (raw file):
state: string; recurrence: string; jobsrunning: number;
on the ScheduleColumns and Schedule types you're using a mix of camel case, sneak case and this one that doesn't differentiate the second word. Make sure you pick one style and use it consistently
pkg/ui/workspaces/cluster-ui/src/api/schedulesApi.ts line 109 at r3 (raw file):
// may also be truncated. sql: ` WITH schedules AS (SHOW SCHEDULES)
looks like you're using this to get the info for the details page, but is the same query you did to get all info, just filter by the specific one. When you get all, that is saved on cache, so instead of having to make another call, you can simply look for the result on your cache.
Take a look at this example, which is very similar, on it we have an overview with all statements, we don't make any new calls and on the selector for the details you just do a find on the cache:
83ae32f#diff-8d9a8801f6be479b8728f6010c8a626e6e3d243c850a40815e592c7e75a54dcfL55-L66
pkg/ui/workspaces/cluster-ui/src/schedules/scheduleDetailsPage/scheduleDetails.tsx line 64 at r3 (raw file):
<Row gutter={24}> <Col className="gutter-row" span={24}> <SqlBox value={schedule.command} />
nit: I added a new size type that we're trying to use now, so if you rebase, change this to <SqlBox value={schedule.command} size={SqlBoxSize.custom} />
pkg/ui/workspaces/db-console/src/redux/apiReducers.ts line 441 at r3 (raw file):
export const scheduleKey = (scheduleID: Long): string => scheduleID.toString(); const scheduleReducerObj = new KeyedCachedDataReducer(
with the suggestion I mention, you won't need this cache specific per schedule
There was a problem hiding this comment.
Regarding the code itself and the changes requested,
Now it's up to the others tagged here, as Kevin mentioned, if there are changes on design, location of page or content.
Reviewable status:
There was a problem hiding this comment.
Reviewable status:
pkg/ui/workspaces/cluster-ui/src/api/schedulesApi.ts line 23 at r3 (raw file):
Previously, maryliag (Marylia Gutierrez) wrote…
on the ScheduleColumns and Schedule types you're using a mix of camel case, sneak case and this one that doesn't differentiate the second word. Make sure you pick one style and use it consistently
discussed offline, needs to match the sql columns
pkg/ui/workspaces/cluster-ui/src/api/schedulesApi.ts line 109 at r3 (raw file):
Previously, maryliag (Marylia Gutierrez) wrote…
looks like you're using this to get the info for the details page, but is the same query you did to get all info, just filter by the specific one. When you get all, that is saved on cache, so instead of having to make another call, you can simply look for the result on your cache.
Take a look at this example, which is very similar, on it we have an overview with all statements, we don't make any new calls and on the selector for the details you just do a
findon the cache:
83ae32f#diff-8d9a8801f6be479b8728f6010c8a626e6e3d243c850a40815e592c7e75a54dcfL55-L66
discussed offline, probably not right for this use case
pkg/ui/workspaces/cluster-ui/src/schedules/scheduleDetailsPage/scheduleDetails.tsx line 64 at r3 (raw file):
Previously, maryliag (Marylia Gutierrez) wrote…
nit: I added a new size type that we're trying to use now, so if you rebase, change this to
<SqlBox value={schedule.command} size={SqlBoxSize.custom} />
done
pkg/ui/workspaces/db-console/src/redux/apiReducers.ts line 441 at r3 (raw file):
Previously, maryliag (Marylia Gutierrez) wrote…
with the suggestion I mention, you won't need this cache specific per schedule
discussed offline, probably not right for this use cas
|
Thanks for looping me in @benbardin and @kevin-v-ngo ! The new Schedules view is neat!
I have the same opinion - if Schedules and Jobs are related, ideally they are consolidated in a single page and separated by tabs (similar to the SQL Activity page). Do you think consolidating them together will happen in time for the public release of v22.2? I assumed so, but wanted to clarify that in case I'm misinterpreting your point around "revise for a later release". Just to share some context behind one of my concerns around not having them consolidated them in time for v22.2 - if the first time users see a new "Schedules" top-level nav item, and then in a subsequent patch release that nav item disappears (b/c it is consolidated with something else), that shifting information hierarchy could lead to user confusion. And we'd like to avoid that where possible :) |
|
Thanks @thtruo ! I don't think I can do better than this before the branch cut Tuesday (or even before 22.2 GA), not least because we may want visual designs for a consolidated tab that I'm not skilled enough to make :) I think our choices are either ship this for 22.2 and revise for 23.1 as need/resources indicate, or choose not to ship this now. I bias towards shipping, but certainly agree we should be thoughtful around cognitive load. What do you think - "go" or "no go," here? Or is there another option I'm missing? |
|
Ack @benbardin |
|
@benbardin @thtruo - there is enough user-facing value in shipping this for 22.2 as-is. I vote ship-it. Re: consolidation - we can have further discussions with @jeremycbloom and align across other product areas to see if consolidating all "jobs" makes sense from a user perspective for a future iteration. |
Adds /schedules page in DB console. Structure is very similar to /jobs pa ge. The new /schedules page provides a bit more observability into what schedules are running, and their various states. Release note (ui change): Add page to view schedules in DB console. Release justification: Low-risk - new, read-only page.
|
Test failure is unrelated bors r+ |
|
would having something like this enough for the change? https://www.loom.com/share/134c2296a97a4eb5941cf54a5d162a63 To clarify, this would only show like this for DB Console, CC Console would not be affected by this change (since the schedules page won't exist there, meaning only Jobs page show there) |
|
If it helps I create the PR #86945 |
|
Build failed (retrying...): |
|
Build failed (retrying...): |
|
Build succeeded: |
Summary
Adds /schedules page in DB console. Structure is very similar to /jobs page. The new /schedules page provides a bit more observability into what schedules are running, and their various states.
Release note (ui change): Add page to view schedules in DB console.
Release justification: Low-risk - new, read-only page.
See also #70725
Artifacts
Loom link: https://www.loom.com/share/0fb4fee5859d4a46a3b6442892ae9ef6