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.

Sign up for GitHub

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

Jump to bottom

Improve performance of showing partial Pipelines and runs #1452

Merged
merged 1 commit into from
Jul 14, 2023
Merged

Improve performance of showing partial Pipelines and runs #1452

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 12 additions & 2 deletions frontend/src/api/pipelines/custom.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,13 @@ export const listExperiments: ListExperimentsAPI = (hostPath) => (opts) =>
export const listPipelines: ListPipelinesAPI = (hostPath) => (opts, count) =>
handlePipelineFailures(
// eslint-disable-next-line camelcase
proxyGET(hostPath, '/apis/v1beta1/pipelines', { page_size: count }, opts),
proxyGET(
hostPath,
'/apis/v1beta1/pipelines',
// eslint-disable-next-line camelcase
{ page_size: count, sort_by: 'created_at desc' },
opts,
),
);

export const listPipelineRuns: ListPipelinesRunAPI = (hostPath) => (opts) =>
Expand All @@ -77,14 +83,18 @@ export const listPipelineRunJobs: ListPipelinesRunJobAPI = (hostPath) => (opts)
handlePipelineFailures(proxyGET(hostPath, '/apis/v1beta1/jobs', {}, opts));

export const listPipelineRunsByPipeline: ListPipelineRunsByPipelineAPI =
(hostPath) => (opts, pipelineId) =>
(hostPath) => (opts, pipelineId, count) =>
handlePipelineFailures(
proxyGET(
hostPath,
'/apis/v1beta1/runs',
{
'resource_reference_key.id': pipelineId,
'resource_reference_key.type': ResourceTypeKF.PIPELINE_VERSION,
// eslint-disable-next-line camelcase
page_size: count,
// eslint-disable-next-line camelcase
sort_by: 'created_at desc',
},
opts,
),
Expand Down
7 changes: 4 additions & 3 deletions frontend/src/concepts/pipelines/apiHooks/usePipelineRunsForPipeline.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,14 @@ import useFetchState, { FetchStateCallbackPromise } from '~/utilities/useFetchSt
import { usePipelinesAPI } from '~/concepts/pipelines/context';
import { POLL_INTERVAL } from '~/utilities/const';

const usePipelineRunsForPipeline = (pipeline: PipelineKF) => {
const usePipelineRunsForPipeline = (pipeline: PipelineKF, limit: number) => {
const { api } = usePipelinesAPI();

const pipelineId = pipeline.id;
const call = React.useCallback<FetchStateCallbackPromise<PipelineRunKF[]>>(
(opts) => api.listPipelineRunsByPipeline(opts, pipelineId).then(({ runs }) => runs ?? []),
[api, pipelineId],
(opts) =>
api.listPipelineRunsByPipeline(opts, pipelineId, limit).then(({ runs }) => runs ?? []),
[api, pipelineId, limit],
);

return useFetchState(call, [], { refreshRate: POLL_INTERVAL });
Expand Down
2 changes: 2 additions & 0 deletions frontend/src/concepts/pipelines/const.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,2 +1,4 @@
export const PIPELINE_ROUTE_NAME = 'ds-pipeline-pipelines-definition';
export const PIPELINE_DEFINITION_NAME = 'pipelines-definition';
export const TABLE_CONTENT_LIMIT = 5;
export const LIMIT_MAX_ITEM_COUNT = TABLE_CONTENT_LIMIT + 1;
1 change: 0 additions & 1 deletion frontend/src/concepts/pipelines/content/tables/pipeline/PipelinesTable.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@ const PipelinesTable: React.FC<PipelinesTableProps> = ({
{...tableProps}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two things here...

First, spreads usually are always first unless you're looking to have "defaults" that are overridden by the spread values.

<Component
  // defaults -- usually not done in most cases
  {...data} // other values that are not likely in here, or optionals for overriding defaults
  // explicit values -- will override anything passed in `data`
/>

We want to make sure nothing that we "want" is before a spread -- it will override that state.

Second, enablePagination here is also not ideal -- this table is shared by two callers:

  • Global Pipelines
  • Project Pipelines

Global Pipelines wants to be enabled, and is if you look at PipelinesView

Project Pipelines does not want to paginate -- since we are truncating the values to less than the default page limit, it won't ever trigger, but this is not great.

Changes needed

  • Please remove the enablePagination from this shared component, let the spread handle that if the caller wants it.
  • Move spread back to the top, there is no need to try and override these values (data & enablePagination -- which you are removing)

data={pipelines}
columns={pipelineColumns}
defaultSortColumn={1}
variant={TableVariant.compact}
truncateRenderingAt={contentLimit}
rowRenderer={(pipeline, rowIndex) => (
Expand Down
7 changes: 5 additions & 2 deletions frontend/src/concepts/pipelines/content/tables/pipeline/PipelinesTableExpandedRow.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import { PipelineKF, PipelineRunKF } from '~/concepts/pipelines/kfTypes';
import IndentSection from '~/pages/projects/components/IndentSection';
import { FetchState } from '~/utilities/useFetchState';
import { usePipelinesAPI } from '~/concepts/pipelines/context';
import { TABLE_CONTENT_LIMIT } from '~/concepts/pipelines/const';
import { RenderContentList, combineRunsByColumn } from './expandedRowRenderUtils';

type PipelinesTableExpandedRowProps = {
Expand Down Expand Up @@ -75,7 +76,7 @@ const PipelinesTableExpandedRow: React.FC<PipelinesTableExpandedRowProps> = ({
);
}

const renderContentByColumn = combineRunsByColumn(runs, 5);
const renderContentByColumn = combineRunsByColumn(runs, TABLE_CONTENT_LIMIT);

return (
<Tbody isExpanded={isExpanded}>
Expand All @@ -92,7 +93,9 @@ const PipelinesTableExpandedRow: React.FC<PipelinesTableExpandedRowProps> = ({
}
items={[
...renderContentByColumn.names,
runs.length > 5 && <Link to={`/pipelineRuns/${namespace}`}>View all runs</Link>,
runs.length > TABLE_CONTENT_LIMIT && (
<Link to={`/pipelineRuns/${namespace}`}>View all runs</Link>
),
]}
/>
</IndentSection>
Expand Down
3 changes: 2 additions & 1 deletion frontend/src/concepts/pipelines/content/tables/pipeline/PipelinesTableRow.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import {
RunNameForPipeline,
RunStatus,
} from '~/concepts/pipelines/content/tables/renderUtils';
import { LIMIT_MAX_ITEM_COUNT } from '~/concepts/pipelines/const';

type PipelinesTableRowProps = {
pipeline: PipelineKF;
Expand All @@ -32,7 +33,7 @@ const PipelinesTableRow: React.FC<PipelinesTableRowProps> = ({
}) => {
const navigate = useNavigate();
const { namespace } = usePipelinesAPI();
const runsFetchState = usePipelineRunsForPipeline(pipeline);
const runsFetchState = usePipelineRunsForPipeline(pipeline, LIMIT_MAX_ITEM_COUNT);
const [isExpanded, setExpanded] = React.useState(false);

const createdDate = new Date(pipeline.created_at);
Expand Down
3 changes: 1 addition & 2 deletions frontend/src/concepts/pipelines/content/tables/pipeline/expandedRowRenderUtils.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ import {
RunNameForPipeline,
RunStatus,
} from '~/concepts/pipelines/content/tables/renderUtils';
import { sortRunsByCreated } from '~/concepts/pipelines/content/tables/utils';

type RenderContentListProps = {
firstItem?: React.ReactNode;
Expand All @@ -30,7 +29,7 @@ type RenderContentByColumn = {
};

export const combineRunsByColumn = (runs: PipelineRunKF[], sliceCount?: number) =>
sortRunsByCreated(runs).reduce<RenderContentByColumn>(
runs.reduce<RenderContentByColumn>(
(acc, run, i) => {
if (sliceCount && i >= sliceCount) {
return acc;
Expand Down
5 changes: 1 addition & 4 deletions frontend/src/concepts/pipelines/content/tables/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,7 @@ import {
} from '~/concepts/pipelines/kfTypes';
import { DateRangeString, splitDateRange } from '~/components/dateRange/utils';

export const sortRunsByCreated = (runs: PipelineRunKF[]) =>
[...runs].sort((a, b) => new Date(b.created_at).getTime() - new Date(a.created_at).getTime());

export const getLastRun = (runs: PipelineRunKF[]) => sortRunsByCreated(runs)[0];
export const getLastRun = (runs: PipelineRunKF[]) => runs[0];

export const getRunDuration = (run: PipelineRunKF): number => {
const finishedDate = new Date(run.finished_at);
Expand Down
1 change: 1 addition & 0 deletions frontend/src/concepts/pipelines/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ export type ListPipelineRunJobs = (opts: K8sAPIOptions) => Promise<ListPipelineR
export type ListPipelineRunsByPipeline = (
opts: K8sAPIOptions,
pipelineId: string,
limit?: number,
) => Promise<ListPipelineRunsResourceKF>;
export type ListPipelineTemplates = (
opts: K8sAPIOptions,
Expand Down
9 changes: 4 additions & 5 deletions frontend/src/pages/projects/screens/detail/pipelines/PipelinesList.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,11 @@ import usePipelines from '~/concepts/pipelines/apiHooks/usePipelines';
import IndentSection from '~/pages/projects/components/IndentSection';
import { usePipelinesAPI } from '~/concepts/pipelines/context';
import EmptyStateErrorMessage from '~/components/EmptyStateErrorMessage';

const CONTENT_LIMIT = 5;
import { TABLE_CONTENT_LIMIT, LIMIT_MAX_ITEM_COUNT } from '~/concepts/pipelines/const';

const PipelinesList: React.FC = () => {
const { namespace } = usePipelinesAPI();
const [pipelines, loaded, loadError, refresh] = usePipelines();
const [pipelines, loaded, loadError, refresh] = usePipelines(LIMIT_MAX_ITEM_COUNT);
const navigate = useNavigate();

if (loadError) {
Expand Down Expand Up @@ -40,12 +39,12 @@ const PipelinesList: React.FC = () => {
<StackItem>
<PipelinesTable
pipelines={pipelines}
contentLimit={CONTENT_LIMIT}
contentLimit={TABLE_CONTENT_LIMIT}
pipelineDetailsPath={(namespace, id) => `/projects/${namespace}/pipeline/view/${id}`}
refreshPipelines={refresh}
/>
</StackItem>
{pipelines.length > CONTENT_LIMIT && (
{pipelines.length > TABLE_CONTENT_LIMIT && (
<StackItem>
<IndentSection>
<Button variant="link" onClick={() => navigate(`/pipelines/${namespace}`)}>
Expand Down
Loading