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

server, ui: remove interpreted jobs retrying status #97505

Merged
merged 2 commits into from
Feb 27, 2023

Conversation

xinhaoz
Copy link
Member

@xinhaoz xinhaoz commented Feb 22, 2023

This commit removes the 'Retrying' status from the jobs UX.
Previously, we were interpolating this status from the running
status. This just added confusion and incorectness to the status
of the job being displayed. The status being surfaced now aligns
directly with what is shown in the crdb_internal.jobs table.

Some missing job statuses were also added as request options to
the 'Status' dropdown, including:

  • Pause Requested
  • Cancel Requested
  • Revert Failed

Fixes: #95712

Release note (ui change): Retrying is no longer a status shown
in the jobs page.

image

Reformats query string for the jobs admin endpoint.

Epic: none

Release note: None
@xinhaoz xinhaoz requested review from a team February 22, 2023 19:28
@xinhaoz xinhaoz requested a review from a team as a code owner February 22, 2023 19:28
@xinhaoz xinhaoz requested a review from a team February 22, 2023 19:28
@cockroach-teamcity
Copy link
Member

This change is Reviewable

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.

Just one small nit and a question, but feel free to ignore if doesn't make sense

:lgtm:

Reviewed 1 of 1 files at r1, 5 of 6 files at r2, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @xinhaoz)


pkg/ui/workspaces/cluster-ui/src/jobs/jobsPage/jobsPage.tsx line 155 at r2 (raw file):

  componentDidUpdate(prevProps: JobsPageProps): void {
    // (xzhang) Because we removed the retrying status, we add this check

nit: you don't need to add your name when is not a TODO


pkg/ui/workspaces/cluster-ui/src/jobs/util/jobOptions.tsx line 63 at r2 (raw file):

export function isRunning(status: string): boolean {
  return status.includes(JOB_STATUS_RUNNING);

I think the reverting was also being consider so it shows a progress bar. Should a reverting status be added here since is a type of "running"?

@xinhaoz xinhaoz force-pushed the jobs-retry-status-fix branch from e12cae6 to 5589dd0 Compare February 27, 2023 14:20
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 (and 1 stale) (waiting on @maryliag)


pkg/ui/workspaces/cluster-ui/src/jobs/jobsPage/jobsPage.tsx line 155 at r2 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

nit: you don't need to add your name when is not a TODO

Removed.


pkg/ui/workspaces/cluster-ui/src/jobs/util/jobOptions.tsx line 63 at r2 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

I think the reverting was also being consider so it shows a progress bar. Should a reverting status be added here since is a type of "running"?

Ah gotcha - included it.

This commit removes the 'Retrying' status from the jobs UX.
Previously, we were interpolating this status from the running
status. This just added confusion and incorectness to the status
of the job being displayed. The status being surfaced now aligns
directly with what is shown in the `crdb_internal.jobs` table.

Some missing job statuses were also added as request options to
the 'Status' dropdown, including:
- Pause Requested
- Cancel Requested
- Revert Failed

Fixes: cockroachdb#95712

Release note (ui change): Retrying is no longer a status shown
in the jobs page.
@xinhaoz xinhaoz force-pushed the jobs-retry-status-fix branch from 5589dd0 to 2eca521 Compare February 27, 2023 14:51
@xinhaoz
Copy link
Member Author

xinhaoz commented Feb 27, 2023

TFTR!
bors r+

@craig
Copy link
Contributor

craig bot commented Feb 27, 2023

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Feb 27, 2023

Build succeeded:

@craig craig bot merged commit f1a4c63 into cockroachdb:master Feb 27, 2023
@xinhaoz xinhaoz deleted the jobs-retry-status-fix branch June 5, 2023 17:04
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.

sql: admin server gives scheduled GC job "Retrying" status
3 participants