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

feat(fcm): Implement sendEach, sendEachAsync, sendEachForMulticast and sendEachForMulticastAsync (#785) #815

Merged
merged 6 commits into from
Jun 8, 2023

Conversation

Doris-Ge
Copy link
Contributor

@Doris-Ge Doris-Ge commented Jun 2, 2023

  • Add org.hamcrest as dependency for unit tests

  • Implement sendEach, sendEachAsync, sendEachForMulticast and sendEachForMulticastAsync

sendEach vs sendAll

  1. sendEach sends one HTTP request to V1 Send endpoint for each message in the array. sendAll sends only one HTTP request to V1 Batch Send endpoint to send all messages in the array.
  2. sendEach calls messagingClient.send to send each message and constructs a SendResponse with the returned messageId. If messagingClient.send throws out an exception, sendEach will catch the exception and also turn it into a SendResponse with the exception in it. sendEach calls ApiFutures.allAsList().get() to execute all messagingClient.send calls asynchronously and wait for all of them to complete and construct a BatchResponse with all SendResponses. Therefore, unlike sendAll, sendEach does not always throw an error for a total failure. It can also return a BatchResponse with only errors in it.

sendEachForMulticast calls sendEach under the hood. sendEachAsync is the async version of sendEach. sendEachForMulticastAsync is the async version of sendEachForMulticast.

  • Add integration tests for batch-send re-implementation: testSendEach(), testSendFiveHundredWithSendEach(), testSendEachForMulticast()

RELEASE NOTE: sendAll(), sendAllAsync(), sendMulticast(), and sendMulticastAsync() APIs are now deprecated. Use sendEach(), sendEachAsync(), sendEachForMulticast(), and sendEachForMulticastAsync() APIs instead.

…st` and `sendEachForMulticastAsync` (#785)

* Add org.hamcrest as dependency for unit tests

* Implement sendEach, sendEachAsync, sendEachForMulticast and sendEachForMulticastAsync

`sendEach` vs `sendAll`
1. `sendEach` sends one HTTP request to V1 Send endpoint for each
    message in the array.
   `sendAll` sends only one HTTP request to V1 Batch Send endpoint
    to send all messages in the array.
2. `sendEach` calls `messagingClient.send` to send each message
    and constructs a `SendResponse` with the returned `messageId`.
    If `messagingClient.send` throws out an exception, `sendEach`
    will catch the exception and also turn it into a `SendResponse`
    with the exception in it.
    `sendEach` calls `ApiFutures.allAsList().get()` to execute all
    `messagingClient.send` calls asynchronously and wait for all of
    them to complete and construct a `BatchResponse` with all
    `SendResponse`s.
    Therefore, unlike `sendAll`, `sendEach` does not always throw
    an error for a total failure. It can also return a `BatchResponse`
    with only errors in it.

`sendEachForMulticast` calls `sendEach` under the hood.
`sendEachAsync` is the async version of `sendEach`.
`sendEachForMulticastAsync` is the async version of `sendEachForMulticast`.

* Add integration tests for batch-send re-implementation:
testSendEach(), testSendFiveHundredWithSendEach(), testSendEachForMulticast()
@lahirumaramba lahirumaramba added release-note release:stage Stage a release candidate labels Jun 2, 2023
Copy link
Member

@lahirumaramba lahirumaramba left a comment

Choose a reason for hiding this comment

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

Thank you!
Please get the docs reviewed if we haven't done so already in the fcm-batch-send branch.

@Doris-Ge Doris-Ge requested a review from egilmorez June 2, 2023 21:03
Copy link
Contributor

@egilmorez egilmorez left a comment

Choose a reason for hiding this comment

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

A few style things for you Doris! Otherwise, LGTM from a doc string perspective, thanks!

@Doris-Ge
Copy link
Contributor Author

Doris-Ge commented Jun 8, 2023

Kevin and Eric, thank you both for reviewing the PR! All the comments have been resolved. Merging this PR to the branch.

@Doris-Ge Doris-Ge closed this Jun 8, 2023
@Doris-Ge Doris-Ge reopened this Jun 8, 2023
@Doris-Ge Doris-Ge merged commit d1baee3 into master Jun 8, 2023
@Doris-Ge Doris-Ge deleted the fcm-batch-send branch June 8, 2023 20:10
@grodri93
Copy link

What new method is equivalent to sendMulticastMessage?

@KrishnakanthYachareni
Copy link

Hi @Doris-Ge @lahirumaramba ,
You mentioned that sendEach sends one HTTP request to V1 Send endpoint for each message in the array. sendAll sends only one HTTP request to V1 Batch Send endpoint to send all messages in the array. But when I compare both the calls, found that both are invoking same number of HTTP calls.

Could you please provide your inputs to the folllowing questions.

  1. I have tried to understand the difference between the sendEachAsync and sendEachForMulticastAsync methods in terms of performance. However, both methods seem to make the same number of REST calls to FCM based on the number of device tokens in the message. Can you clarify this further?
  2. Is there a method similar to the BATCH endpoint where only one REST call is made for multiple device tokens?
  3. Once we upgrade to the latest version of firebase-admin (9.2.0) from our current version (7.0.1), will there be any performance improvements in terms of the number of REST calls made to FCM? Are there any other benefits to upgrading to the latest version with respect to the client application? (Aside from the deprecated batch endpoint, are there any other changes?)

I appreciate your response.

@Doris-Ge
Copy link
Contributor Author

Hi @KrishnakanthYachareni,

  1. Yes. That's correct.
  2. No. The only ones are the deprecated sendAll and sendMulticast methods, but they will stop working once the batch endpoint is shut down in June, 2024.
  3. The reason we made the changes to the APIs is because FCM is shutting down the batch endpoint in June, 2024. It's expected that there will be no performance improvements.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:stage Stage a release candidate release-note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants