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 Other types of campaigns to projectBySlug webservice #1136

Merged

Conversation

mohammadranjbarz
Copy link
Collaborator

@mohammadranjbarz mohammadranjbarz marked this pull request as ready for review September 27, 2023 13:45
Copy link
Collaborator

@CarlosQ96 CarlosQ96 left a comment

Choose a reason for hiding this comment

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

Mohammad getAllProjectsRelatedToActiveCampaigns I think we could improve the inner query and loops a bit. I think we could potentially load too much in memory. Wdyt?

Aside from that other tests/code looks good.

break;
}
const projectsQuery = filterProjectsQuery(projectsQueryParams);
const projects = await projectsQuery.getMany();
Copy link
Collaborator

@CarlosQ96 CarlosQ96 Sep 28, 2023

Choose a reason for hiding this comment

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

I think this is a dangerous query, we could have multiple active campaigns and a campaign might contain many many projects (specially filter type campaigns). This will load a lot of information PER REQUEST in memory, we had issues before where loading too much can block the request.

And its also inside a double loop.

I suggest we reduce the complexity by making a similar query to this one but passing the ID of the project we need specifically (the minimal project).

Since projectBySlug query returns only 1 item. I don't think its necessary to load everything into memory every time. Wdty?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Even with the cache, every 10 minutes it could be a big memory spike.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I dont know how we can solve it with a simple query, if there is something in your mind definitely it would be better, because for campaigns that uses filter or sort, the included projects may change during the time so if want to know a project belongs to which campaign we should check all campaign's projects and it would decrease performance especially projectBySlug is one of webservices that frontend call it almost many times.

And campaigns that uses filter and sort just have 10 projects so the cached object would not be a large object so I thought maybe it's better to store it in memory instead of redis ( because it's already complicated this part, I though it may make it more complicated

src/resolvers/projectResolver.test.ts Outdated Show resolved Hide resolved
Copy link
Collaborator

@CarlosQ96 CarlosQ96 left a comment

Choose a reason for hiding this comment

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

I think we can move forward so QA can keep testing. Tests show it all looks good!!
Thanks!

My only suggestion is if there is any weird results or issues we use the methods inside redis.ts file, and cache using redis -> Which is less prone to data corruption/errors. We can do it in another Pr if its required.

WDYT? @mohammadranjbarz
Can merge meanwhile.

@mohammadranjbarz
Copy link
Collaborator Author

I think we can move forward so QA can keep testing. Tests show it all looks good!! Thanks!

My only suggestion is if there is any weird results or issues we use the methods inside redis.ts file, and cache using redis -> Which is less prone to data corruption/errors. We can do it in another Pr if its required.

WDYT? @mohammadranjbarz Can merge meanwhile.

Thank @CarlosQ96 , you did a really good job on reviewing this PR, I appreciate it

@mohammadranjbarz mohammadranjbarz merged commit 05eeae0 into staging Oct 3, 2023
3 checks passed
@mohammadranjbarz mohammadranjbarz deleted the f_1051_fix_add_campaigns_to_project_by_slug branch October 3, 2023 08:21
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