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

Add pagination for oscar referrals #131

Open
wants to merge 4 commits into
base: staging
Choose a base branch
from
Open

Conversation

mtuchi
Copy link
Collaborator

@mtuchi mtuchi commented Jul 2, 2024

Description

Add ability to paginate oscar cases. Since we are fetching cases with referrals i have limit the pagination to 20 cases per request because in each 20 cases we are making 20 request to fetch case referrals.

These changes have been test using the following versions

╭─────────────────────────────────────────────╮
│ ◲ ◱  @openfn/core#v1.5.1 (Node.js v18.12.1) │
│ ◳ ◰         @openfn/[email protected] │
╰─────────────────────────────────────────────╯

Ref #130

@josephjclark
Copy link

@mtuchi this branch is in conflict with master

@josephjclark
Copy link

@mtuchi I'm not sure I understand the reasoning behind fetching 20 cases at a time.

Whether you fetch 20 or 1000 at once, you're still going to make a separate request for each individual case. So what benefit does a smaller batch have?

@josephjclark
Copy link

@mtuchi also this problem was true on the original version - the base branch will still send out a new request for every referral. All we've done in this PR is is make MORE Api calls. Significantly more if we're pulling down ~1000 records.

@mtuchi
Copy link
Collaborator Author

mtuchi commented Jul 3, 2024

@josephjclark withReferrals option is the reason for creating a small batch because it will fetch referrals for each case.
If you have 20 cases it will make 20 request to fetch referrals. So if we use 1000 and get 1000 cases back then it will make 1000 request to fetch referrals.

Making a small batch can help with retry incase of failure because you can adjust the currentPage and retry from there

@josephjclark
Copy link

Ok, the debugging argument seems valid. I'm just interested in how much slower this change is going to make it.

Again, in the v1 implementation, it just fetched everything and then fetched referrals one at a time and extra debugging wasn't needed then 🤷

Maybe a bigger page, perhaps 100 cases, would be a better compromise?

One thing I would recommend is that you add logging in the job code at the start and end of each page

@mtuchi mtuchi force-pushed the 130-cases-pagination branch from 7c10855 to 0e88356 Compare July 4, 2024 06:50
@mtuchi
Copy link
Collaborator Author

mtuchi commented Jul 4, 2024

@josephjclark fixed the git conflict
I have updated the perPage to 100 and add logs before and after each getCases

@mtuchi mtuchi requested a review from josephjclark July 4, 2024 06:51
@mtuchi mtuchi requested a review from josephjclark July 5, 2024 07:48
@mtuchi mtuchi changed the base branch from master to staging July 15, 2024 12:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants