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

Handle GCM reflections in batches. #513

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

balbesina
Copy link

@balbesina balbesina commented Jul 31, 2019

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.

# example usage
Rpush.configure do |config|
  config.gcm_batch_reflections = true
end
Rpush.reflect do |on|
  on.gcm_delivered_to_recipients do |notification, successes_map|
    success_values = successes_map.keys.map { |token| [notification.id, token] }
    DeviceDeliveryStatus.import %i[notification_id device_token], success_values
  end
end

@aried3r
Copy link
Member

aried3r commented Aug 1, 2019

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.

@balbesina
Copy link
Author

I'll find some time on the upcoming weekend for that.

@balbesina
Copy link
Author

Added same option for notifications: :notification_batch_reflections
Added spec coverage.

@balbesina
Copy link
Author

Please provide acceptable resolution for rubocop's "block too long" of delivery_spec:

  1. add disable comment for block length in file
  2. extend block length for spec folder in .rubocop.yml
  3. extract new examples to separate delivery2_spec

@aried3r
Copy link
Member

aried3r commented Feb 21, 2020

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
Copy link
Contributor

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.

@stale
Copy link

stale bot commented Oct 2, 2021

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.
Thank you for all your contributions!

@stale stale bot added the stale label Oct 2, 2021
@aried3r aried3r removed the stale label Oct 8, 2021
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

3 participants