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: cockroachdb#95712 Release note (ui change): Retrying is no longer a status shown in the jobs page.
xinhaoz · Feb 22, 2023 · e12cae6 · e12cae6
1 parent 3ae8a88
commit e12cae6
Show file tree

Hide file tree

Showing 6 changed files with 69 additions and 98 deletions.
diff --git 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
@@ -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<JobsPageProps, PageState> {
     }
 
     // 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<JobsPageProps, PageState> {
   }
 
   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<JobsPageProps, PageState> {
             <PageConfigItem>
               <Dropdown items={statusOptions} onChange={this.onStatusSelected}>
                 Status:{" "}
-                {
-                  statusOptions.find(option => option["value"] === status)[
-                    "name"
-                  ]
-                }
+                {statusOptions.find(option => option.value === status)?.name}
               </Dropdown>
             </PageConfigItem>
             <PageConfigItem>
               <Dropdown items={typeOptions} onChange={this.onTypeSelected}>
                 Type:{" "}
                 {
-                  typeOptions.find(
-                    option => option["value"] === type.toString(),
-                  )["name"]
+                  typeOptions.find(option => option.value === type.toString())
+                    ?.name
                 }
               </Dropdown>
             </PageConfigItem>
             <PageConfigItem>
               <Dropdown items={showOptions} onChange={this.onShowSelected}>
-                Show:{" "}
-                {showOptions.find(option => option["value"] === show)["name"]}
+                Show: {showOptions.find(option => option.value === show)?.name}
               </Dropdown>
             </PageConfigItem>
           </PageConfig>

diff --git 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,36 +55,41 @@ 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);
 }
 
 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
@@ -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<JobStatusProps> = ({
         </div>
       );
     case JobStatusVisual.ProgressBarWithDuration: {
-      const jobIsRetrying = isRetrying(job.status);
       return (
         <div>
           <ProgressBar
@@ -63,7 +62,6 @@ export const JobStatus: React.FC<JobStatusProps> = ({
             showPercentage={true}
           />
           <Duration job={job} className={cx("jobs-table__duration")} />
-          {jobIsRetrying && <RetryingStatusBadge />}
           {job.running_status && (
             <div className={cx("jobs-table__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
@@ -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<JobStatusCellProps> = ({
   lineWidth,
   compact = false,
   hideDuration = false,
-}) => {
-  const jobStatus = (
-    <JobStatus
-      job={job}
-      lineWidth={lineWidth}
-      compact={compact}
-      hideDuration={hideDuration}
-    />
-  );
-  if (isRetrying(job.status)) {
-    return (
-      <Tooltip
-        placement="bottom"
-        style="tableTitle"
-        content={
-          <>
-            Next Planned Execution Time:
-            <br />
-            {TimestampToMoment(job.next_run).format(DATE_FORMAT_24_UTC)}
-          </>
-        }
-      >
-        {jobStatus}
-      </Tooltip>
-    );
-  }
-  return jobStatus;
-};
+}) => (
+  <JobStatus
+    job={job}
+    lineWidth={lineWidth}
+    compact={compact}
+    hideDuration={hideDuration}
+  />
+);
diff --git 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 <Badge status={badgeStatus} text={badgeText} />;
+    return <Badge status={badgeStatus} text={jobStatus} />;
   }
 }