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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[4.x]馃悰 Fixed member exports timing out for large sites #14876

Merged

Conversation

SimonBackx
Copy link
Contributor

@codecov
Copy link

codecov bot commented May 20, 2022

Codecov Report

Merging #14876 (9770f85) into 4.x (e5ff14f) will decrease coverage by 0.09%.
The diff coverage is 9.67%.

@@            Coverage Diff             @@
##              4.x   #14876      +/-   ##
==========================================
- Coverage   59.81%   59.72%   -0.10%     
==========================================
  Files         580      581       +1     
  Lines       48038    48121      +83     
  Branches     4220     4221       +1     
==========================================
+ Hits        28735    28739       +4     
- Misses      19262    19341      +79     
  Partials       41       41              
Impacted Files Coverage 螖
core/server/services/members/exporter/query.js 6.81% <6.81%> (酶)
...ver/api/canary/utils/serializers/output/members.js 90.03% <33.33%> (+0.81%) 猬嗭笍
core/server/services/members/service.js 42.30% <100.00%> (酶)

Continue to review full report at Codecov.

Legend - Click here to learn more
螖 = absolute <relative> (impact), 酶 = not affected, ? = missing data
Powered by Codecov. Last update e5ff14f...9770f85. Read the comment docs.

@SimonBackx SimonBackx force-pushed the improved-members-export-performance-4-x branch 2 times, most recently from e8ebd4e to e3fd191 Compare May 20, 2022 13:05
@SimonBackx SimonBackx force-pushed the improved-members-export-performance-4-x branch from e3fd191 to 00b6757 Compare May 20, 2022 13:06
@SimonBackx SimonBackx marked this pull request as ready for review May 20, 2022 13:44
Copy link
Member

@ErisDS ErisDS 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 this can be merged - I have minor cosmetic comments.

const page = await models.Member.findPage(options);
ids = page.data.map(d => d.id);

/*
Copy link
Member

Choose a reason for hiding this comment

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

Could you add a comment line explaining why this is commented out?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I tried last minute to improve the id fetching. But it didn't work reliably (I think filtering with search didn't work). I still think the commented code would have better performance if that is fixed because it 'escapes' from bookshelf. So I quickly commented it out before switching branches to work on the tests for #14873 (switching between 4.x and 5.x takes a bit longer with the client/admin rename). Could have stashed the changes, but then I might forget about it 馃槵

@@ -1554,7 +1554,7 @@ describe('Members API', function () {
'content-disposition': anyString
});

res.text.should.match(/id,email,name,note,subscribed_to_emails,complimentary_plan,stripe_customer_id,created_at,deleted_at/);
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason to keep these 2 tests in members.test.js when we've got a suite in members-exporter.test.js now? Seems like they are duplicates?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't have any tests yet for the search option in the other file, so I think it is best to keep this one until we have a new one for search

Copy link
Contributor Author

Choose a reason for hiding this comment

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

One of them is no search, I just noticed. We can clean it up later :)

@SimonBackx SimonBackx merged commit 61aa30b into TryGhost:4.x May 20, 2022
@SimonBackx SimonBackx deleted the improved-members-export-performance-4-x branch May 20, 2022 16:46
ErisDS pushed a commit to ErisDS/Ghost that referenced this pull request May 20, 2022
refs TryGhost/Product#1641

This commit adds a custom query for the members export, to improve the performance and to prevent any timeouts from happening when exporting large amounts of members.
ErisDS added a commit that referenced this pull request May 20, 2022
refs TryGhost/Product#1641

This commit adds a custom query for the members export, to improve the performance and to prevent any timeouts from happening when exporting large amounts of members.

Co-authored-by: Simon Backx <simon@ghost.org>
Co-authored-by: Matt Hanley <git@matthanley.co.uk>
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.

None yet

2 participants