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.

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

fix: apply custom cursor pagination where workflows and archived workflows are merged #11761

Conversation

sunyeongchoi
Copy link
Member

Fixes #11715

Motivation

Workflows and archive workflows use different pagination logics.
However, in the current workflow page, if both workflows and archive workflows coexist, we want them to be retrieved together.
The issue arises from passing the pagination options used for workflows pagination directly to archive workflows, causing a mismatch between the types used in archive workflows and workflows, resulting in archive workflows not being retrieved.

Modifications

I have implemented the following logic to resolve this issue:

  1. fetch the entire list of workflows and archived workflows separately.
  2. merge the workflows and archived workflows.
  3. perform cursor pagination on the merged result based on the resourceVersion.

After contemplating on how to address this issue, I concluded that the conventional approach of paginating workflows and archived workflows separately would not easily resolve this problem. Therefore, I implemented it in this manner.

Verification

  • Confirming the pagination is functioning correctly.
  • Checking if Archive workflows are retrieved successfully.
  • Verifying that Workflows are retrieved correctly.
  • Validating the proper functioning of tests with the 'make test' command.

Copy link
Member

@terrytangyuan terrytangyuan left a comment

Choose a reason for hiding this comment

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

This is great! Could you also look into adding some tests around cursorPaginationByResourceVersion?

@agilgur5 agilgur5 added area/server area/api Argo Server API labels Sep 6, 2023
@sunyeongchoi
Copy link
Member Author

This is great! Could you also look into adding some tests around cursorPaginationByResourceVersion?

Sure! I'll write a test code and upload a commit tomorrow :)

Comment on lines +213 to +214
// Search whole with Limit 0.
// Reset the Continue "" to prevent Kubernetes native pagination.
Copy link
Member

Choose a reason for hiding this comment

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

Would this approach require fetching the entire list every time user clicks another page?

Copy link
Member Author

@sunyeongchoi sunyeongchoi Sep 6, 2023

Choose a reason for hiding this comment

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

Yes, the entire list is fetched every time the page changed.

Copy link
Member Author

Choose a reason for hiding this comment

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

I have written and uploaded test code for cursorPaginationByResourceVersion. 859c0b2

I am concerned about the potential performance impact of my implementation because it retrieves the entire dataset with each page change. However, I can't think of a better way to solve this problem on the server-side other than this method.

In my opinion, unless there's a better approach, the server should send the entire list to the front-end once, and then pagination can be handled on the front-end. However, one drawback of this approach is that the data won't be updated on the front-end without refreshing the page. To achieve real-time data updates, we might need to consider using websockets or polling.

Do you have any suggestions or better ideas on how to handle this?

Copy link
Member

Choose a reason for hiding this comment

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

I don't have a better idea. I think correctness is important right now. Performance-wise, we should do something from the front-end like what you said to only refetch the whole list when refreshing. This can be something that's configurable.

Copy link
Member Author

Choose a reason for hiding this comment

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

Then, after this PR, I will think about the front page pagination and give it a try.

Also, since I am curious about the performance of the currently implemented pagination, I will think about ways to test its performance.

I will share the test results later when they come out.

Copy link
Member

Choose a reason for hiding this comment

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

Great!

@sunyeongchoi sunyeongchoi force-pushed the bufgix/archived-workflows-pagination branch 2 times, most recently from 8713736 to bd50093 Compare September 7, 2023 12:21
server/workflow/workflow_server.go Outdated Show resolved Hide resolved
server/workflow/workflow_server.go Outdated Show resolved Hide resolved
server/workflow/workflow_server.go Outdated Show resolved Hide resolved
server/workflow/workflow_server.go Outdated Show resolved Hide resolved
@terrytangyuan
Copy link
Member

This looks good once the last comment and the conflicts are resolved. Great work! This should be included in the next RC #11381

…hived-workflows-pagination

Signed-off-by: sunyeongchoi <[email protected]>
@terrytangyuan terrytangyuan enabled auto-merge (squash) September 12, 2023 00:57
auto-merge was automatically disabled September 12, 2023 01:33

Head branch was pushed to by a user without write access

@sunyeongchoi sunyeongchoi force-pushed the bufgix/archived-workflows-pagination branch from abfe950 to f1318f6 Compare September 12, 2023 01:34
@terrytangyuan terrytangyuan enabled auto-merge (squash) September 12, 2023 01:55
auto-merge was automatically disabled September 12, 2023 02:28

Head branch was pushed to by a user without write access

@terrytangyuan terrytangyuan enabled auto-merge (squash) September 12, 2023 02:42
@terrytangyuan
Copy link
Member

Need to sort the imports

auto-merge was automatically disabled September 12, 2023 08:21

Head branch was pushed to by a user without write access

@sunyeongchoi sunyeongchoi reopened this Sep 12, 2023
@terrytangyuan terrytangyuan merged commit e073dcc into argoproj:master Sep 12, 2023
42 of 43 checks passed
@terrytangyuan
Copy link
Member

@sunyeongchoi Looks like the test is flaky. See https://github.com/argoproj/argo-workflows/actions/runs/6166062084/job/16734921912

@sunyeongchoi
Copy link
Member Author

@sunyeongchoi Looks like the test is flaky. See https://github.com/argoproj/argo-workflows/actions/runs/6166062084/job/16734921912

Oh no, I'll quickly post a PR fixing that test. sorry.

@sunyeongchoi
Copy link
Member Author

@sunyeongchoi Looks like the test is flaky. See https://github.com/argoproj/argo-workflows/actions/runs/6166062084/job/16734921912

I uploaded bugfix PR about this :) #11816
Thaks for your report.

terrytangyuan added a commit to terrytangyuan/argo-workflows that referenced this pull request Oct 23, 2023
terrytangyuan added a commit to terrytangyuan/argo-workflows that referenced this pull request Oct 23, 2023
terrytangyuan added a commit that referenced this pull request Oct 24, 2023
terrytangyuan added a commit that referenced this pull request Nov 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3.5 Pagination may not work correctly for archived workflows
3 participants