Improve performance of showing partial Pipelines and runs #1452

uidoyen · 2023-06-30T11:30:38Z

Description

Improve performance of showing partial Pipelines / associated Pipeline Runs by enabling pagination and limit the items. Also sort the pipelines and runs by created_at.

How Has This Been Tested?

Test Impact

Project Pipelines & Project Pipelines' Runs

Created few sample pipelines and runs
Go to DS Project -> select project -> Pipelines -> expand pipeline which contains the runs

Global Pipelines:

Global Pipeline scheduled runs:

Global Pipeline triggered runs(zoomed out):

Request review criteria:

The commits have meaningful messages (squashes happen on merge by the bot).
Included any necessary screenshots or gifs if it was a UI change.
Included tags to the UX team if it was a UI/UX change.
Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
The developer has manually tested the changes and verified that the changes work
The developer has added tests or explained why testing cannot be added (unit tests & storybook for related changes)

openshift-ci · 2023-06-30T11:30:49Z

Hi @uidoyen. Thanks for your PR.

I'm waiting for a opendatahub-io member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

manaswinidas · 2023-07-03T12:26:33Z

@uidoyen I think there is linting error, can you fix it?

manaswinidas · 2023-07-04T08:11:35Z

/ok-to-test

lucferbux

I think you are missing the point of the change.
The issue describes that we need to filter the pipelines k8s call but I think part of the goal of this issue is to improve performance by not fetching all the calls right?

frontend/src/concepts/pipelines/content/tables/pipeline/PipelinesTable.tsx

frontend/src/utilities/sortByCreatedAt.ts

lucferbux · 2023-07-06T11:57:54Z

@uidoyen Can you rebase to get latest changes, there's a bug fix for an issue present in this PR (was in our codebase before)

andrewballantyne

There are three views here:

Projects Pipelines
Global Pipelines
Global Runs (scheduled and triggered)

There are two types of changes needed:

Project Pipelines needs to limit to "5" because we fetching 20 has no value since we only want to show 5
Project Pipelines & Global Pipelines show the last 5 runs inside the expanded section -- but fetch 20 and truncate client side

So we need to handle this truncation via API on the Project Pipelines for Pipelines & Runs. Global Pipelines we need to truncate via API for the expanded row's runs. Global Runs does not need to be changed for this issue.

We want to improve our fetching so we can get the "latest" from the API without having to fetch 15 more than we need. We can fetch 6 instead of 5 that we show if that's the easiest way to show the related "show more" links that take you to the Global Pipelines or Global Runs page (depending on what link you click on).

This effort is to just limit fetching in some cases for some resources. We want our Global pages to show as much as it can for that given type.

Hope this all makes sense, let me know if you have any questions.

frontend/src/concepts/pipelines/content/tables/pipeline/PipelinesTableRow.tsx

frontend/src/pages/pipelines/global/pipelines/PipelinesView.tsx

frontend/src/concepts/pipelines/content/tables/pipelineRun/PipelineRunTable.tsx

andrewballantyne · 2023-07-10T17:27:19Z

@uidoyen Can you fix up your screenshots in your description?

andrewballantyne

Couple notes

frontend/src/concepts/pipelines/content/tables/pipeline/PipelinesTableExpandedRow.tsx

frontend/src/const.ts

andrewballantyne

Looks good code-wise and screenshot wise -- I have not tested it.

/lgtm

uidoyen · 2023-07-11T04:29:45Z

@pnaik1 @manaswinidas Can you guys test it please?

pnaik1 · 2023-07-11T05:22:24Z

Okay @uidoyen

pnaik1 · 2023-07-11T09:58:21Z

/lgtm
but the tests are failing

lucferbux · 2023-07-11T11:21:11Z

Im gonna trigger again the checks and review it. Let's see if this time it doesn't fail

lucferbux · 2023-07-11T15:58:26Z

Code looks great, requests have the query params that we wanted, I only have a nit, but I think is fine as it is right now.

As you can see in the response, the request return the total runs that you have, so LIMIT_MAX_ITEM_COUNT is not needed to show expanded, it can be achieved just fetching 5 and then checking if total_size is greater than TABLE_CONTENT_LIMIT.

I can imagine we haven't implemented this since total_size is optional and it might be missing in some scenarios right?

lucferbux · 2023-07-11T15:58:42Z

But im gonna approve it anyways cause code looks great.

lucferbux

/lgtm

lucferbux · 2023-07-11T16:08:23Z

I'm sorry @uidoyen I think you need to rebase interactively your PR to squash all the commits beforehand, we need to do this for ever pr for now own, please do that before merging

pnaik1 · 2023-07-13T10:51:28Z

/lgtm

manaswinidas

The changes work fine for me.

/lgtm

andrewballantyne

/approve

I'm going to add the approval label because the code works. But we should clean up the implementation so it's accurately representing the component in question -- see my comment below.

Get any review from anyone else after you adjust the prop stuff. No need to wait on me unless you feel there are more questions.

andrewballantyne · 2023-07-13T15:58:34Z

frontend/src/concepts/pipelines/content/tables/pipeline/PipelinesTable.tsx

        data={pipelines}
+        enablePagination
+        {...tableProps}


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)

openshift-ci · 2023-07-14T14:24:27Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andrewballantyne, lucferbux, manaswinidas

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [andrewballantyne]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci bot requested review from alexcreasy and lucferbux June 30, 2023 11:30

openshift-ci bot added the needs-ok-to-test The openshift bot needs to label PRs from non members to avoid strain on the CI label Jun 30, 2023

uidoyen force-pushed the odh-1248 branch from 481e081 to 82a01ba Compare July 3, 2023 14:23

openshift-ci bot added ok-to-test The openshift bot needs `ok-to-test` to allow non member PRs to run the tests. and removed needs-ok-to-test The openshift bot needs to label PRs from non members to avoid strain on the CI labels Jul 4, 2023

lucferbux requested changes Jul 6, 2023

View reviewed changes

frontend/src/concepts/pipelines/content/tables/pipeline/PipelinesTable.tsx Outdated Show resolved Hide resolved

frontend/src/utilities/sortByCreatedAt.ts Outdated Show resolved Hide resolved

frontend/src/utilities/sortByCreatedAt.ts Outdated Show resolved Hide resolved

openshift-ci bot assigned lucferbux Jul 6, 2023

uidoyen force-pushed the odh-1248 branch from 5c266f3 to 914edba Compare July 6, 2023 13:36

uidoyen requested a review from lucferbux July 6, 2023 13:37

uidoyen force-pushed the odh-1248 branch 3 times, most recently from 166f12b to 6ce0c3a Compare July 10, 2023 14:47

andrewballantyne requested changes Jul 10, 2023

View reviewed changes

openshift-ci bot assigned andrewballantyne Jul 10, 2023

uidoyen requested a review from andrewballantyne July 10, 2023 17:01

andrewballantyne requested changes Jul 10, 2023

View reviewed changes

frontend/src/concepts/pipelines/content/tables/pipeline/PipelinesTableExpandedRow.tsx Outdated Show resolved Hide resolved

frontend/src/const.ts Outdated Show resolved Hide resolved

uidoyen requested a review from andrewballantyne July 10, 2023 18:03

andrewballantyne reviewed Jul 10, 2023

View reviewed changes

openshift-ci bot added the lgtm label Jul 10, 2023

openshift-ci bot assigned pnaik1 Jul 11, 2023

lucferbux approved these changes Jul 11, 2023

View reviewed changes

lucferbux mentioned this pull request Jul 11, 2023

replace EmptyState to Spinner component in Pipeline runs details tab #1442

Merged

6 tasks

uidoyen force-pushed the odh-1248 branch from 5581898 to f5f5cc0 Compare July 11, 2023 16:53

openshift-ci bot removed the lgtm label Jul 11, 2023

uidoyen force-pushed the odh-1248 branch 2 times, most recently from dfee18b to 9093881 Compare July 11, 2023 17:28

uidoyen requested a review from andrewballantyne July 13, 2023 06:46

openshift-ci bot added the lgtm label Jul 13, 2023

manaswinidas approved these changes Jul 13, 2023

View reviewed changes

openshift-ci bot assigned manaswinidas Jul 13, 2023

andrewballantyne requested changes Jul 13, 2023

View reviewed changes

openshift-ci bot added approved and removed lgtm labels Jul 13, 2023

uidoyen force-pushed the odh-1248 branch from 9093881 to 8166377 Compare July 14, 2023 13:14

Improve performance of showing partial Pipelines and runs

9634ba6

uidoyen force-pushed the odh-1248 branch from 8166377 to 9634ba6 Compare July 14, 2023 13:17

uidoyen requested a review from andrewballantyne July 14, 2023 14:21

andrewballantyne approved these changes Jul 14, 2023

View reviewed changes

openshift-ci bot added the lgtm label Jul 14, 2023

openshift-merge-robot merged commit 225ba23 into opendatahub-io:main Jul 14, 2023
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve performance of showing partial Pipelines and runs #1452

Improve performance of showing partial Pipelines and runs #1452

uidoyen commented Jun 30, 2023 •

edited

Loading

openshift-ci bot commented Jun 30, 2023

manaswinidas commented Jul 3, 2023

manaswinidas commented Jul 4, 2023

lucferbux left a comment

lucferbux commented Jul 6, 2023

andrewballantyne left a comment

andrewballantyne commented Jul 10, 2023

andrewballantyne left a comment

andrewballantyne left a comment

uidoyen commented Jul 11, 2023

pnaik1 commented Jul 11, 2023

pnaik1 commented Jul 11, 2023 •

edited

Loading

lucferbux commented Jul 11, 2023

lucferbux commented Jul 11, 2023

lucferbux commented Jul 11, 2023

lucferbux left a comment

lucferbux commented Jul 11, 2023

pnaik1 commented Jul 13, 2023

manaswinidas left a comment

andrewballantyne left a comment

andrewballantyne Jul 13, 2023

openshift-ci bot commented Jul 14, 2023

Improve performance of showing partial Pipelines and runs #1452

Improve performance of showing partial Pipelines and runs #1452

Conversation

uidoyen commented Jun 30, 2023 • edited Loading

Description

How Has This Been Tested?

Test Impact

Project Pipelines & Project Pipelines' Runs

Request review criteria:

openshift-ci bot commented Jun 30, 2023

manaswinidas commented Jul 3, 2023

manaswinidas commented Jul 4, 2023

lucferbux left a comment

Choose a reason for hiding this comment

lucferbux commented Jul 6, 2023

andrewballantyne left a comment

Choose a reason for hiding this comment

andrewballantyne commented Jul 10, 2023

andrewballantyne left a comment

Choose a reason for hiding this comment

andrewballantyne left a comment

Choose a reason for hiding this comment

uidoyen commented Jul 11, 2023

pnaik1 commented Jul 11, 2023

pnaik1 commented Jul 11, 2023 • edited Loading

lucferbux commented Jul 11, 2023

lucferbux commented Jul 11, 2023

lucferbux commented Jul 11, 2023

lucferbux left a comment

Choose a reason for hiding this comment

lucferbux commented Jul 11, 2023

pnaik1 commented Jul 13, 2023

manaswinidas left a comment

Choose a reason for hiding this comment

andrewballantyne left a comment

Choose a reason for hiding this comment

andrewballantyne Jul 13, 2023

Choose a reason for hiding this comment

Changes needed

openshift-ci bot commented Jul 14, 2023

uidoyen commented Jun 30, 2023 •

edited

Loading

pnaik1 commented Jul 11, 2023 •

edited

Loading