feat(fcm): Implement sendEach, sendEachAsync, sendEachForMulticast and sendEachForMulticastAsync
#785
Conversation
| List<SendResponse> responses = ApiFutures.allAsList(list).get(); | ||
| return new BatchResponseImpl(responses); | ||
| } catch (InterruptedException | ExecutionException e) { | ||
| throw new FirebaseMessagingException(ErrorCode.CANCELLED, SERVICE_ID); |
There was a problem hiding this comment.
We can discuss what error code we want to throw here.
There was a problem hiding this comment.
Also, we can discuss whether we should make an effort to add a unit test for this case
There was a problem hiding this comment.
If one message fails to go through would this throw immediately cancelling the rest of the messages?
There was a problem hiding this comment.
If one message fails because of FirebaseMesssagingException, then it won't cancel the rest of the messages because the exception will be caught in sendOpForSendResponse.
Based on my read of https://dzone.com/articles/google-guava-%E2%80%93-futures, allAsList is fail-fast.
So if one message fails because the execute() throws an exception other than FirebaseMesssagingException,
protected SendResponse execute() {
try {
String messageId = messagingClient.send(message, dryRun);
return SendResponse.fromMessageId(messageId);
} catch (FirebaseMessagingException e) {
return SendResponse.fromException(e);
}
}
then the rest of the messages will be cancelled. I'm not sure in what cases this could happen, but I think it should be a corner case.
There was a problem hiding this comment.
I'm going to merge this PR. Let me know if you have any suggested changes for this. I'll fix it in another PR.
…orMulticastAsync
`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`.
65f4435 to
bd798b5
Compare
testSendEach(), testSendFiveHundredWithSendEach(), testSendEachForMulticast()
sendEach, sendEachAsync, sendEachForMulticast and sendEachForMulticastAsync
| List<SendResponse> responses = ApiFutures.allAsList(list).get(); | ||
| return new BatchResponseImpl(responses); | ||
| } catch (InterruptedException | ExecutionException e) { | ||
| throw new FirebaseMessagingException(ErrorCode.CANCELLED, SERVICE_ID); |
There was a problem hiding this comment.
If one message fails to go through would this throw immediately cancelling the rest of the messages?
…st` and `sendEachForMulticastAsync` (#785) (#815) * feat(fcm): Implement `sendEach`, `sendEachAsync`, `sendEachForMulticast` 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() * Replace all "-- i.e." in the comments with "meaning that" * Fix the build errors caused by "Line is longer than 100 characters" * Fix the build errors caused by "package does not exist" * Address doc review comments
|
Could you please tell a little bit why you deprecated sendAll and recommend us to use sendEach instead? What was the motivation behind it. I know sendEach looks more reliable to debug failed attempts, however, even if it can send http requests async meaning that it may perform as good as sendAll, does it require a single thread from the thread-pool and cause more memory-usage, more networking and less available threads in pool? Therefore, the sendAll method looks more efficient to use threads to me. We are sending millions of notifications in a couple of minutes and we consider to lower our memory usage. Thanks in advance. |
|
Hi @sarismet,
This is because
I'm not sure whether it requires a single thread to send one async request. For lowering memory usage, I'd suggest to reduce the number of messages to send per |
|
Hi @sarismet |
|
Hi @amitkumar43 We overwrite ThreadManager with custom thread executor. It helps a lot. Before that our Kafka consumer gets into rebalancing. Nowadays, we are trying to use virtualThreadExecutors in ThreadManager. We reach 2 millions request per minutes but we got errors saying that "Error writing request body to server", "Remote host terminated the handshake", "Unexpected end of file from server". When load is decreased, the error counts decreases as well. Do you have any idea what causes these errors? Have you ever tried virtual threads in that project? @Doris-Ge @lahirumaramba Our java docker image: eclipse-temurin:21.0.3_9-jre-alpine |
sendEach,sendEachAsync,sendEachForMulticastandsendEachForMulticastAsyncsendAll,sendAllAsync,sendMulticastandsendMulticastAsyncsendEachvssendAllsendEachsends one HTTP request to V1 Send endpoint for eachmessage in the array.
sendAllsends only one HTTP request to V1 Batch Send endpointto send all messages in the array.
sendEachcallsmessagingClient.sendto send each messageand constructs a
SendResponsewith the returnedmessageId.If
messagingClient.sendthrows out an exception,sendEachwill catch the exception and also turn it into a
SendResponsewith the exception in it.
sendEachcallsApiFutures.allAsList().get()to execute allmessagingClient.sendcalls asynchronously and wait for all ofthem to complete and construct a
BatchResponsewith allSendResponses.Therefore, unlike
sendAll,sendEachdoes not always throwan error for a total failure. It can also return a
BatchResponsewith only errors in it.
sendEachForMulticastcallssendEachunder the hood.sendEachAsyncis the async version ofsendEach.sendEachForMulticastAsyncis the async version ofsendEachForMulticast.Will send another PR for integration tests