-
Notifications
You must be signed in to change notification settings - Fork 342
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
Handle GCM reflections in batches. #513
base: master
Are you sure you want to change the base?
Conversation
Hey! Thanks for your PR, I think it makes a lot of sense :) Could you please add tests for it? There's quite a lot going on, new conditionals and changes in behavior. I think it'd be best some tests would document this. |
I'll find some time on the upcoming weekend for that. |
Added same option for notifications: :notification_batch_reflections |
Please provide acceptable resolution for rubocop's "block too long" of delivery_spec:
|
Hey! Sorry I've neglected this PR for a while. @rpush/maintainers, kindly asking for a second pair of eyes for review. |
return if successes.blank? | ||
|
||
if Rpush.config.gcm_batch_reflections | ||
successes_map = successes.map { |result| [result[:registration_id], result[:canonical_id]] }.to_h |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like potential memory issue here and for failures_map
in handle_errors
as well.
successes
can hold thousands of objects and map
will create a new array in memory, the same to_h
does.
You can use something like inject
or each_with_object
in order to reduce the number of iterations and objects but still not sure if it is good idea "double" the memory consumption.
This issue has been automatically marked as stale because it has not had recent activity. If this is still an issue, please leave another comment. This issue will be closed if no further activity occurs. |
Since we push messages to GCM in a batch and the response comes with each registration_id delivery status, there is no need to reflect each device separately. This feature allows to receive all successful and failed statues per single reflection.