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

Retrieve emails as a generator #376

Merged
merged 1 commit into from
Mar 26, 2018
Merged

Conversation

samuelhwilliams
Copy link
Contributor

Summary

We have been getting intermittent 504 timeouts on our DOS email upload
job and they trace back to DMMailChimpClient's
get_email_addresses_from_list job, which tries to get emails back in
batches of 1000. This seems to take too long sometimes, so have reduced
it to a page size of 100 and converted the method to a generator to
reduce overhead.

Ticket

https://trello.com/c/4cK1wTvq/236

Copy link
Contributor

@galund galund left a comment

Choose a reason for hiding this comment

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

Seems like the right change to make anyway, but it feels equally likely to me that a smaller page size / more calls would result in more timeouts, as result in fewer.

@samuelhwilliams
Copy link
Contributor Author

Hopefully not...

@risicle
Copy link
Contributor

risicle commented Mar 26, 2018

We've typically given some kind of hint in the method name when returning an iterator, e.g. the api client's *_iter() methods.

@samuelhwilliams
Copy link
Contributor Author

This is fair enough, although I think reading the method name you'd still expect to be given an iterable back - just from the fact it's plural on email_addresses. Happy enough to rename it if we think this is for the best, but I don't think it'll make the library more consistent with anything. Might need to be done as a "one fell swoop" sort of thing. 🤔

@risicle
Copy link
Contributor

risicle commented Mar 26, 2018

🤷‍♂️


return email_addresses
for member in member_data["members"]:
yield member['email_address']
Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW if you wanted to show off you could do this as a yield from (member['email_address'] for member in member_data["members"])

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Aha. I had a feeling something like this existed but didn't follow through on that feeling to actually look it up.

Thanks

 ## Summary
We have been getting intermittent 504 timeouts on our DOS email upload
job and they trace back to DMMailChimpClient's
get_email_addresses_from_list job, which tries to get emails back in
batches of 1000. This seems to take too long sometimes, so have reduced
it to a page size of 100 and converted the method to a generator to
reduce overhead.

 ## Ticket
https://trello.com/c/4cK1wTvq/236
@samuelhwilliams samuelhwilliams force-pushed the shw-mailchimp-generator branch from 1e14475 to efa02ba Compare March 26, 2018 09:11
@samuelhwilliams samuelhwilliams merged commit a49b62d into master Mar 26, 2018
@samuelhwilliams samuelhwilliams deleted the shw-mailchimp-generator branch March 26, 2018 10:00
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.

3 participants