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): Add SendEachAsync and SendEachForMulticastAsync for FCM batch send #348

Merged
merged 2 commits into from
Jun 9, 2023

Conversation

Doris-Ge
Copy link
Contributor

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

  • Deprecate SendAllAsync() and SendMulticastAsync APIs.
  • Add SendEachAsync and SendEachForMulticastAsync APIs

SendEachAsync vs SendAllAsync

  1. SendEachAsync sends one HTTP request to V1 Send endpoint for each message in the list. SendAllAsync sends only one HTTP request to V1 Batch Send endpoint to send all messages in the list.
  2. SendEachAsync calls Task.WhenAll to wait for all httpClient.SendAndDeserializeAsync calls to complete and construct a BatchResponse with all SendResponses. An httpClient.SendAndDeserializeAsync call to V1 Send endpoint either completes with a success or throws an exception. So if an exception is thrown out, the exception will be caught in SendEachAsync and turned into a SendResponse with an exception. Therefore, unlike SendAllAsync, SendEachAsync does not always throw an exception for a total failure. It can also return a BatchResponse with only exceptions in it.

SendEachForMulticastAsync calls SendEachAsync under the hood.

RELEASE NOTE: SendAllAsync() and SendMulticastAsync() APIs are now deprecated. Use SendEachAsync() and SendEachForMulticastAsync() APIs instead.

…343)

* Implement `SendEachAsync` and `SendEachForMulticastAsync`

`SendEachAsync` vs `SendAllAsync`
1. `SendEachAsync` sends one HTTP request to V1 Send endpoint for each
    message in the list.
   `SendAllAsync` sends only one HTTP request to V1 Batch Send endpoint
    to send all messages in the list.
2. `SendEachAsync` calls `Task.WhenAll` to wait for all
    `httpClient.SendAndDeserializeAsync` calls to complete and construct
    a `BatchResponse` with all `SendResponse`s.
    An `httpClient.SendAndDeserializeAsync` call to V1 Send endpoint
    either completes with a success or throws an exception. So if an
    exception is thrown out, the exception will be caught in `SendEachAsync`
    and turned into a `SendResponse` with an exception.
    Therefore, unlike `SendAllAsync`, `SendEachAsync` does not always throw
    an exception for a total failure. It can also return a `BatchResponse`
    with only exceptions in it.

`SendEachForMulticastAsync` calls `SendEachAsync` under the hood.

* Add integration tests for the batch-send reimplementation:
SendEach() and SendEachForMulticast()
@Doris-Ge Doris-Ge requested a review from lahirumaramba June 8, 2023 06:12
FirebaseAdmin/FirebaseAdmin/Messaging/FirebaseMessaging.cs Outdated Show resolved Hide resolved
/// </summary>
/// <exception cref="FirebaseMessagingException">If an error occurs while sending the
/// messages.</exception>
/// <param name="messages">Up to 500 messages to send in the batch. Cannot be null.</param>
Copy link
Collaborator

Choose a reason for hiding this comment

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

empty ok?

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'm not sure what you mean. Could you clarify?

Copy link
Collaborator

Choose a reason for hiding this comment

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

non-null and non-empty? or just non null?

Copy link
Contributor Author

@Doris-Ge Doris-Ge Jun 8, 2023

Choose a reason for hiding this comment

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

I think empty is also not ok. It's the same for SendAllAsync, but we didn't explicitly mention this for SendAllAsync in the past. Should I change it to "Cannot be null or empty." for both all SendAll APIs and all SendEach APIs? Or should I keep SendAll API as they are now and only update the comment for SendEach APIs?

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 kept SendAll API unchanged and updated to "Cannot be null or empty." for all SendEach APIs.

FirebaseAdmin/FirebaseAdmin/Messaging/FirebaseMessaging.cs Outdated Show resolved Hide resolved
FirebaseAdmin/FirebaseAdmin/Messaging/FirebaseMessaging.cs Outdated Show resolved Hide resolved
FirebaseAdmin/FirebaseAdmin/Messaging/FirebaseMessaging.cs Outdated Show resolved Hide resolved
@lahirumaramba lahirumaramba changed the title feat(fcm): SendEachAsync and SendEachForMulticastAsync for FCM batch send feat(fcm): Add SendEachAsync and SendEachForMulticastAsync for FCM batch send Jun 9, 2023
@lahirumaramba lahirumaramba added release-note release:stage Stage a release candidate labels Jun 9, 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!

@Doris-Ge Doris-Ge merged commit 74bd9e5 into master Jun 9, 2023
@hankovich
Copy link

@Doris-Ge Could you please clarify why sending N requests with 1 notification in each is better rather then sending 1 request with N notifications? For me that fact that I should use SendEachAsync instead of SendAllAsync sounds counterintuitive

@Doris-Ge
Copy link
Contributor Author

@hankovich SendAllAsync calls the legacy Batch Send API which will be discontinued starting June 20, 2024. That's the main reason why it is replaced by SendEachAsync. SendEachAsynccalls the HTTP v1 API which does not support sending 1 request with N notifications.

@naveedahmed1
Copy link

Do we have similar option available in .net sdk? firebase/firebase-admin-node#2488 (comment)

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.

5 participants