From b8bcc4837894cdf3159505e44e0302716ceeb8a4 Mon Sep 17 00:00:00 2001 From: Stephen Smith Date: Fri, 21 Jun 2024 14:18:30 -0400 Subject: [PATCH] 21925: Don't pass full resource to progress bar useEffect to stop request spam * Changes progress bar fetch to accept UUID * Split polling flag into 2 for process and project * Fetch method now only returns polling flag for projects * The previous changes for project support switched to updating isRunning in the fetch action where it would use the store for processes and progress counts for projects * This resulted in polling not starting if a draft process is already open when started since it would only update if already polling * This new approach uses the store (including WS updates) for processes and polling-based updates for projects * Splitting these up also allows eliminating an extra refresh when project polling stops while keeping a final refresh when websocket updates a process to stopped * Added comments and better type guards Arvados-DCO-1.1-Signed-off-by: Stephen Smith --- .../subprocess-progress-bar.tsx | 57 ++++++++++----- .../subprocess-panel-actions.ts | 69 +++++++++++++------ 2 files changed, 89 insertions(+), 37 deletions(-) diff --git a/services/workbench2/src/components/subprocess-progress-bar/subprocess-progress-bar.tsx b/services/workbench2/src/components/subprocess-progress-bar/subprocess-progress-bar.tsx index 78df83f6ca..27a1c7b5e6 100644 --- a/services/workbench2/src/components/subprocess-progress-bar/subprocess-progress-bar.tsx +++ b/services/workbench2/src/components/subprocess-progress-bar/subprocess-progress-bar.tsx @@ -6,10 +6,10 @@ import React, { useEffect, useState } from "react"; import { StyleRulesCallback, Tooltip, WithStyles, withStyles } from "@material-ui/core"; import { CProgressStacked, CProgress } from '@coreui/react'; import { useAsyncInterval } from "common/use-async-interval"; -import { Process } from "store/processes/process"; +import { Process, isProcessRunning } from "store/processes/process"; import { connect } from "react-redux"; import { Dispatch } from "redux"; -import { fetchProcessProgressBarStatus } from "store/subprocess-panel/subprocess-panel-actions"; +import { fetchProcessProgressBarStatus, isProcess } from "store/subprocess-panel/subprocess-panel-actions"; import { ProcessStatusFilter, serializeOnlyProcessTypeFilters } from "store/resource-type-filters/resource-type-filters"; import { ProjectResource } from "models/project"; import { getDataExplorer } from "store/data-explorer/data-explorer-reducer"; @@ -40,7 +40,7 @@ export interface ProgressBarDataProps { } export interface ProgressBarActionProps { - fetchProcessProgressBarStatus: (parentResource: Process | ProjectResource, typeFilter?: string) => Promise; + fetchProcessProgressBarStatus: (parentResourceUuid: string, typeFilter?: string) => Promise; } type ProgressBarProps = ProgressBarDataProps & ProgressBarActionProps & WithStyles; @@ -54,7 +54,7 @@ export type ProgressBarCounts = { export type ProgressBarStatus = { counts: ProgressBarCounts; - isRunning: boolean; + shouldPollProject: boolean; }; const mapStateToProps = (state: RootState, props: ProgressBarDataProps) => { @@ -70,8 +70,8 @@ const mapStateToProps = (state: RootState, props: ProgressBarDataProps) => { }; const mapDispatchToProps = (dispatch: Dispatch): ProgressBarActionProps => ({ - fetchProcessProgressBarStatus: (parentResource: Process | ProjectResource, typeFilter?: string) => { - return dispatch(fetchProcessProgressBarStatus(parentResource, typeFilter)); + fetchProcessProgressBarStatus: (parentResourceUuid: string, typeFilter?: string) => { + return dispatch(fetchProcessProgressBarStatus(parentResourceUuid, typeFilter)); }, }); @@ -79,31 +79,54 @@ export const SubprocessProgressBar = connect(mapStateToProps, mapDispatchToProps ({ parentResource, typeFilter, classes, fetchProcessProgressBarStatus }: ProgressBarProps) => { const [progressCounts, setProgressData] = useState(undefined); - const [isRunning, setIsRunning] = useState(false); - + const [shouldPollProject, setShouldPollProject] = useState(false); + const shouldPollProcess = isProcess(parentResource) ? isProcessRunning(parentResource) : false; + + // Should polling be active based on container status + // or result of aggregated project process contents + const shouldPoll = shouldPollProject || shouldPollProcess; + + const parentUuid = parentResource + ? isProcess(parentResource) + ? parentResource.containerRequest.uuid + : parentResource.uuid + : ""; + + // Runs periodically whenever polling should be happeing + // Either when the workflow is running (shouldPollProcess) or when the + // project contains steps in an active state (shouldPollProject) useAsyncInterval(async () => { - if (parentResource) { - fetchProcessProgressBarStatus(parentResource, typeFilter) + if (parentUuid) { + fetchProcessProgressBarStatus(parentUuid, typeFilter) .then(result => { if (result) { setProgressData(result.counts); - setIsRunning(result.isRunning); + setShouldPollProject(result.shouldPollProject); } }); } - }, isRunning ? 5000 : null); - + }, shouldPoll ? 5000 : null); + + // Runs fetch on first load for processes and projects, except when + // process is running since polling will be enabled by shouldPoll. + // Project polling starts false so this is still needed for project + // initial load to set shouldPollProject and kick off shouldPoll + // Watches shouldPollProcess but not shouldPollProject + // * This runs a final fetch when process ends & is updated through + // websocket / store + // * We ignore shouldPollProject entirely since it changes to false + // as a result of a fetch so the data is already up to date useEffect(() => { - if (!isRunning && parentResource) { - fetchProcessProgressBarStatus(parentResource, typeFilter) + if (!shouldPollProcess && parentUuid) { + fetchProcessProgressBarStatus(parentUuid, typeFilter) .then(result => { if (result) { setProgressData(result.counts); - setIsRunning(result.isRunning); + setShouldPollProject(result.shouldPollProject); } }); } - }, [fetchProcessProgressBarStatus, isRunning, parentResource, typeFilter]); + }, [fetchProcessProgressBarStatus, shouldPollProcess, parentUuid, typeFilter]); let tooltip = ""; if (progressCounts) { diff --git a/services/workbench2/src/store/subprocess-panel/subprocess-panel-actions.ts b/services/workbench2/src/store/subprocess-panel/subprocess-panel-actions.ts index a3d5c68b62..e44c15ba24 100644 --- a/services/workbench2/src/store/subprocess-panel/subprocess-panel-actions.ts +++ b/services/workbench2/src/store/subprocess-panel/subprocess-panel-actions.ts @@ -9,8 +9,11 @@ import { bindDataExplorerActions } from 'store/data-explorer/data-explorer-actio import { FilterBuilder, joinFilters } from 'services/api/filter-builder'; import { ProgressBarStatus, ProgressBarCounts } from 'components/subprocess-progress-bar/subprocess-progress-bar'; import { ProcessStatusFilter, buildProcessStatusFilters } from 'store/resource-type-filters/resource-type-filters'; -import { Process, isProcessRunning } from 'store/processes/process'; +import { Process } from 'store/processes/process'; import { ProjectResource } from 'models/project'; +import { getResource } from 'store/resources/resources'; +import { ContainerRequestResource } from 'models/container-request'; +import { Resource } from 'models/resource'; export const SUBPROCESS_PANEL_ID = "subprocessPanel"; export const SUBPROCESS_ATTRIBUTES_DIALOG = 'subprocessAttributesDialog'; @@ -50,12 +53,29 @@ type ProgressBarStatusPair = { processStatus: ProcessStatusFilter; }; -const isProcess = (resource: Process | ProjectResource | undefined): resource is Process => { +/** + * Type guard to distinguish Processes from other Resources + * @param resource The item to check + * @returns if the resource is a Process + */ +export const isProcess = (resource: T | Process | undefined): resource is Process => { return !!resource && 'containerRequest' in resource; }; -export const fetchProcessProgressBarStatus = (parentResource: Process | ProjectResource, typeFilter?: string) => +/** + * Type guard to distinguish ContainerRequestResources from Resources + * @param resource The item to check + * @returns if the resource is a ContainerRequestResource + */ +const isContainerRequest = (resource: T | ContainerRequestResource | undefined): resource is ContainerRequestResource => { + return !!resource && 'containerUuid' in resource; +}; + +export const fetchProcessProgressBarStatus = (parentResourceUuid: string, typeFilter?: string) => async (dispatch: Dispatch, getState: () => RootState, services: ServiceRepository): Promise => { + const resources = getState().resources; + const parentResource = getResource(parentResourceUuid)(resources); + const requestContainerStatusCount = async (fb: FilterBuilder) => { return await services.containerRequestService.list({ limit: 0, @@ -64,18 +84,21 @@ export const fetchProcessProgressBarStatus = (parentResource: Process | ProjectR }); } - let baseFilter = ""; - if (isProcess(parentResource)) { - baseFilter = new FilterBuilder().addEqual('requesting_container_uuid', parentResource.containerRequest.containerUuid).getFilters(); - } else { + let baseFilter: string = ""; + if (isContainerRequest(parentResource) && parentResource.containerUuid) { + // Prevent CR without containerUuid from generating baseFilter + baseFilter = new FilterBuilder().addEqual('requesting_container_uuid', parentResource.containerUuid).getFilters(); + } else if (parentResource && !isContainerRequest(parentResource)) { + // isCR type narrowing needed since CR without container may fall through baseFilter = new FilterBuilder().addEqual('owner_uuid', parentResource.uuid).getFilters(); } - if (typeFilter) { - baseFilter = joinFilters(baseFilter, typeFilter); - } + if (parentResource && baseFilter) { + // Add type filters from consumers that want to sync progress stats with filters + if (typeFilter) { + baseFilter = joinFilters(baseFilter, typeFilter); + } - if (baseFilter) { try { // Create return object let result: ProgressBarCounts = { @@ -103,17 +126,23 @@ export const fetchProcessProgressBarStatus = (parentResource: Process | ProjectR result[singleResult.status] += singleResult.count; }); - let isRunning = result[ProcessStatusFilter.RUNNING] + result[ProcessStatusFilter.QUEUED] > 0; - - if (isProcess(parentResource)) { - isRunning = isProcessRunning(parentResource); - } - - return {counts: result, isRunning}; + // CR polling is handled in progress bar based on store updates + // This bool triggers polling without causing a final fetch when disabled + // The shouldPoll logic here differs slightly from shouldPollProcess: + // * Process gets websocket updates through the store so using isProcessRunning + // ignores Queued + // * In projects, we get no websocket updates on CR state changes so we treat + // Queued processes as running in order to let polling keep us up to date + // when anything transitions to Running. This also means that a project with + // CRs in a stopped state won't start polling if CRs are started elsewhere + const shouldPollProject = isContainerRequest(parentResource) + ? false + : (result[ProcessStatusFilter.RUNNING] + result[ProcessStatusFilter.QUEUED]) > 0; + + return {counts: result, shouldPollProject}; } catch (e) { return undefined; } - } else { - return undefined; } + return undefined; }; -- 2.30.2