From e12cae691b4f30e7e939ab48d87d1dad5069234a Mon Sep 17 00:00:00 2001 From: Xin Hao Zhang Date: Tue, 21 Feb 2023 13:42:46 -0500 Subject: [PATCH] server, ui: remove interpreted jobs retrying status 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. --- pkg/server/admin.go | 24 +++----- .../cluster-ui/src/jobs/jobsPage/jobsPage.tsx | 38 ++++++++----- .../cluster-ui/src/jobs/util/jobOptions.tsx | 56 +++++++++---------- .../cluster-ui/src/jobs/util/jobStatus.tsx | 4 +- .../src/jobs/util/jobStatusCell.tsx | 40 +++---------- .../cluster-ui/src/jobs/util/progressBar.tsx | 5 +- 6 files changed, 69 insertions(+), 98 deletions(-) diff --git a/pkg/server/admin.go b/pkg/server/admin.go index b581eb160db6..a0ebe8f15e45 100644 --- a/pkg/server/admin.go +++ b/pkg/server/admin.go @@ -2245,8 +2245,6 @@ func jobsHelper( cfg *BaseConfig, sv *settings.Values, ) (_ *serverpb.JobsResponse, retErr error) { - retryRunningCondition := "status='running' AND next_run > now() AND num_runs > 1" - retryRevertingCondition := "status='reverting' AND next_run > now() AND num_runs > 1" q := makeSQLQuery() q.Append(` @@ -2257,11 +2255,7 @@ SELECT statement, user_name, descriptor_ids, - case - when ` + retryRunningCondition + ` then 'retry-running' - when ` + retryRevertingCondition + ` then 'retry-reverting' - else status - end as status, + status, running_status, created, started, @@ -2277,25 +2271,23 @@ SELECT coordinator_id FROM crdb_internal.jobs WHERE true`) // Simplifies filter construction below. - if req.Status == "retrying" { - q.Append(" AND ( ( " + retryRunningCondition + " ) OR ( " + retryRevertingCondition + " ) )") - } else if req.Status != "" { + if req.Status != "" { q.Append(" AND status = $", req.Status) } if req.Type != jobspb.TypeUnspecified { q.Append(" AND job_type = $", req.Type.String()) } else { // Don't show automatic jobs in the overview page. - q.Append(" AND (") + q.Append(" AND ( job_type NOT IN (") for idx, jobType := range jobspb.AutomaticJobTypes { - q.Append("job_type != $", jobType.String()) - if idx < len(jobspb.AutomaticJobTypes)-1 { - q.Append(" AND ") + if idx != 0 { + q.Append(", ") } + q.Append("$", jobType.String()) } - q.Append(" OR job_type IS NULL)") + q.Append(" ) OR job_type IS NULL)") } - q.Append("ORDER BY created DESC") + q.Append(" ORDER BY created DESC") if req.Limit > 0 { q.Append(" LIMIT $", tree.DInt(req.Limit)) } diff --git a/pkg/ui/workspaces/cluster-ui/src/jobs/jobsPage/jobsPage.tsx b/pkg/ui/workspaces/cluster-ui/src/jobs/jobsPage/jobsPage.tsx index 0b2c4528032e..fc63fc66ac49 100644 --- a/pkg/ui/workspaces/cluster-ui/src/jobs/jobsPage/jobsPage.tsx +++ b/pkg/ui/workspaces/cluster-ui/src/jobs/jobsPage/jobsPage.tsx @@ -26,7 +26,14 @@ import { Pagination, ResultsPerPageLabel } from "src/pagination"; import { isSelectedColumn } from "src/columnsSelector/utils"; import { DATE_FORMAT_24_UTC, syncHistory, TimestampToMoment } from "src/util"; import { jobsColumnLabels, JobsTable, makeJobsColumns } from "./jobsTable"; -import { showOptions, statusOptions, typeOptions } from "../util"; +import { + showOptions, + statusOptions, + typeOptions, + isValidJobStatus, + defaultRequestOptions, + isValidJobType, +} from "../util"; import { commonStyles } from "src/common"; import sortableTableStyles from "src/sortedtable/sortedtable.module.scss"; @@ -108,8 +115,8 @@ export class JobsPage extends React.Component { } // Filter Status. - const status = searchParams.get("status") || undefined; - if (this.props.setStatus && status && status != this.props.status) { + const status = searchParams.get("status"); + if (this.props.setStatus && status && status !== this.props.status) { this.props.setStatus(status); } @@ -145,6 +152,17 @@ export class JobsPage extends React.Component { } componentDidUpdate(prevProps: JobsPageProps): void { + // (xzhang) Because we removed the retrying status, we add this check + // just in case there exists an app that attempts to load a non-existent + // status. + if (!isValidJobStatus(this.props.status)) { + this.onStatusSelected(defaultRequestOptions.status); + } + + if (!isValidJobType(this.props.type)) { + this.onTypeSelected(defaultRequestOptions.type.toString()); + } + if ( prevProps.lastUpdated !== this.props.lastUpdated || prevProps.show !== this.props.show || @@ -274,27 +292,21 @@ export class JobsPage extends React.Component { Status:{" "} - { - statusOptions.find(option => option["value"] === status)[ - "name" - ] - } + {statusOptions.find(option => option.value === status)?.name} Type:{" "} { - typeOptions.find( - option => option["value"] === type.toString(), - )["name"] + typeOptions.find(option => option.value === type.toString()) + ?.name } - Show:{" "} - {showOptions.find(option => option["value"] === show)["name"]} + Show: {showOptions.find(option => option.value === show)?.name} diff --git a/pkg/ui/workspaces/cluster-ui/src/jobs/util/jobOptions.tsx b/pkg/ui/workspaces/cluster-ui/src/jobs/util/jobOptions.tsx index f51fafceed29..721a1bac3016 100644 --- a/pkg/ui/workspaces/cluster-ui/src/jobs/util/jobOptions.tsx +++ b/pkg/ui/workspaces/cluster-ui/src/jobs/util/jobOptions.tsx @@ -33,12 +33,8 @@ export function jobToVisual(job: Job): JobStatusVisual { return JobStatusVisual.BadgeWithErrorMessage; case JOB_STATUS_RUNNING: return JobStatusVisual.ProgressBarWithDuration; - case JOB_STATUS_RETRY_RUNNING: - return JobStatusVisual.ProgressBarWithDuration; case JOB_STATUS_PENDING: return JobStatusVisual.BadgeWithMessage; - case JOB_STATUS_RETRY_REVERTING: - return JobStatusVisual.BadgeWithRetrying; case JOB_STATUS_CANCELED: case JOB_STATUS_CANCEL_REQUESTED: case JOB_STATUS_PAUSED: @@ -59,19 +55,12 @@ export const JOB_STATUS_CANCEL_REQUESTED = "cancel-requested"; export const JOB_STATUS_PAUSED = "paused"; export const JOB_STATUS_PAUSE_REQUESTED = "paused-requested"; export const JOB_STATUS_RUNNING = "running"; -export const JOB_STATUS_RETRY_RUNNING = "retry-running"; export const JOB_STATUS_PENDING = "pending"; export const JOB_STATUS_REVERTING = "reverting"; export const JOB_STATUS_REVERT_FAILED = "revert-failed"; -export const JOB_STATUS_RETRY_REVERTING = "retry-reverting"; -export function isRetrying(status: string): boolean { - return [JOB_STATUS_RETRY_RUNNING, JOB_STATUS_RETRY_REVERTING].includes( - status, - ); -} export function isRunning(status: string): boolean { - return [JOB_STATUS_RUNNING, JOB_STATUS_RETRY_RUNNING].includes(status); + return status.includes(JOB_STATUS_RUNNING); } export function isTerminalState(status: string): boolean { return [JOB_STATUS_SUCCEEDED, JOB_STATUS_FAILED].includes(status); @@ -79,16 +68,28 @@ export function isTerminalState(status: string): boolean { export const statusOptions = [ { value: "", name: "All" }, - { value: "succeeded", name: "Succeeded" }, - { value: "failed", name: "Failed" }, - { value: "paused", name: "Paused" }, - { value: "canceled", name: "Canceled" }, - { value: "running", name: "Running" }, - { value: "pending", name: "Pending" }, - { value: "reverting", name: "Reverting" }, - { value: "retrying", name: "Retrying" }, + { value: JOB_STATUS_SUCCEEDED, name: "Succeeded" }, + { value: JOB_STATUS_FAILED, name: "Failed" }, + { value: JOB_STATUS_PAUSED, name: "Paused" }, + { value: JOB_STATUS_PAUSE_REQUESTED, name: "Pause Requested" }, + { value: JOB_STATUS_CANCELED, name: "Canceled" }, + { value: JOB_STATUS_CANCEL_REQUESTED, name: "Cancel Requested" }, + { value: JOB_STATUS_RUNNING, name: "Running" }, + { value: JOB_STATUS_PENDING, name: "Pending" }, + { value: JOB_STATUS_REVERTING, name: "Reverting" }, + { value: JOB_STATUS_REVERT_FAILED, name: "Revert Failed" }, ]; +const ALL_JOB_STATUSES = new Set(statusOptions.map(option => option.value)); + +/** + * @param jobStatus job status - any string + * @returns Returns true if the job status string is a valid status. + */ +export function isValidJobStatus(jobStatus: string): boolean { + return ALL_JOB_STATUSES.has(jobStatus); +} + export function jobHasOneOfStatuses(job: Job, ...statuses: string[]): boolean { return statuses.indexOf(job.status) !== -1; } @@ -110,21 +111,10 @@ export const jobStatusToBadgeStatus = (status: string): BadgeStatus => { case JOB_STATUS_PAUSED: case JOB_STATUS_PAUSE_REQUESTED: case JOB_STATUS_REVERTING: - case JOB_STATUS_RETRY_REVERTING: default: return "default"; } }; -export const jobStatusToBadgeText = (status: string): string => { - switch (status) { - case JOB_STATUS_RETRY_REVERTING: - return JOB_STATUS_REVERTING; - case JOB_STATUS_RETRY_RUNNING: - return JOB_STATUS_RUNNING; - default: - return status; - } -}; const jobTypeKeys = Object.keys(JobType); @@ -216,6 +206,10 @@ export const typeOptions = [ }, ]; +export function isValidJobType(jobType: number): boolean { + return jobType >= 0 && jobType < jobTypeKeys.length; +} + export const showOptions = [ { value: "50", name: "Latest 50" }, { value: "0", name: "All" }, diff --git a/pkg/ui/workspaces/cluster-ui/src/jobs/util/jobStatus.tsx b/pkg/ui/workspaces/cluster-ui/src/jobs/util/jobStatus.tsx index efdfb379710d..30c25a96a333 100644 --- a/pkg/ui/workspaces/cluster-ui/src/jobs/util/jobStatus.tsx +++ b/pkg/ui/workspaces/cluster-ui/src/jobs/util/jobStatus.tsx @@ -13,7 +13,7 @@ import classNames from "classnames/bind"; import React from "react"; import { Duration } from "./duration"; -import { JobStatusVisual, isRetrying, jobToVisual } from "./jobOptions"; +import { JobStatusVisual, jobToVisual } from "./jobOptions"; import { JobStatusBadge, ProgressBar, @@ -54,7 +54,6 @@ export const JobStatus: React.FC = ({ ); case JobStatusVisual.ProgressBarWithDuration: { - const jobIsRetrying = isRetrying(job.status); return (
= ({ showPercentage={true} /> - {jobIsRetrying && } {job.running_status && (
{job.running_status} diff --git a/pkg/ui/workspaces/cluster-ui/src/jobs/util/jobStatusCell.tsx b/pkg/ui/workspaces/cluster-ui/src/jobs/util/jobStatusCell.tsx index 6eef6bec96db..5cb9deac075b 100644 --- a/pkg/ui/workspaces/cluster-ui/src/jobs/util/jobStatusCell.tsx +++ b/pkg/ui/workspaces/cluster-ui/src/jobs/util/jobStatusCell.tsx @@ -8,13 +8,9 @@ // by the Apache License, Version 2.0, included in the file // licenses/APL.txt. import { cockroach } from "@cockroachlabs/crdb-protobuf-client"; -import { Tooltip } from "@cockroachlabs/ui-components"; import React from "react"; -import { TimestampToMoment } from "src/util"; -import { DATE_FORMAT_24_UTC } from "src/util/format"; import { JobStatus } from "./jobStatus"; -import { isRetrying } from "./jobOptions"; type Job = cockroach.server.serverpb.IJobResponse; @@ -30,31 +26,11 @@ export const JobStatusCell: React.FC = ({ lineWidth, compact = false, hideDuration = false, -}) => { - const jobStatus = ( - - ); - if (isRetrying(job.status)) { - return ( - - Next Planned Execution Time: -
- {TimestampToMoment(job.next_run).format(DATE_FORMAT_24_UTC)} - - } - > - {jobStatus} - - ); - } - return jobStatus; -}; +}) => ( + +); diff --git a/pkg/ui/workspaces/cluster-ui/src/jobs/util/progressBar.tsx b/pkg/ui/workspaces/cluster-ui/src/jobs/util/progressBar.tsx index a8d057fbb7c1..56a320d24fdf 100644 --- a/pkg/ui/workspaces/cluster-ui/src/jobs/util/progressBar.tsx +++ b/pkg/ui/workspaces/cluster-ui/src/jobs/util/progressBar.tsx @@ -12,7 +12,7 @@ import { Line } from "rc-progress"; import React from "react"; import { Badge } from "src/badge"; -import { jobStatusToBadgeStatus, jobStatusToBadgeText } from "./jobOptions"; +import { jobStatusToBadgeStatus } from "./jobOptions"; import styles from "../jobs.module.scss"; import classNames from "classnames/bind"; @@ -25,8 +25,7 @@ export class JobStatusBadge extends React.PureComponent<{ jobStatus: string }> { render(): React.ReactElement { const jobStatus = this.props.jobStatus; const badgeStatus = jobStatusToBadgeStatus(jobStatus); - const badgeText = jobStatusToBadgeText(jobStatus); - return ; + return ; } }