search

firebase / firebase-admin-java

Firebase Admin Java SDK
https://firebase.google.com/docs/admin/setup
Apache License 2.0
538 stars 264 forks source link

feat: Add HTTP/2 enabled transport as default transport #979

Open jonathanedey opened 1 month ago

jonathanedey commented 1 month ago

This changes the default HttpTransport to an HTTP/2 enabled Apache 5 HttpClient.

emindeniz99 commented 1 month ago

Thank you for your efforts. I think, if a new transport is implemented and used instead of tested built-in google http client, they should be very well tested. we can copy test cases from this not merged PR to this PR in order to test new http2transport layer.

https://github.com/googleapis/google-http-java-client/pull/1960/files#diff-f6e7e17e639b08337930ac900026abfe1531b7c7e9bea3b6866835526ed1fa81

https://github.com/googleapis/google-http-java-client/pull/1960

emindeniz99 commented 1 month ago

It fixes one of the big problem with current state of sdk but it is could be efficient if thread per request structure of library is changed. CallOp causes blocking a thread per request.

This is better, it does not open many sockets, etc. HTTP2 is used. It solves our main problem. But the fcm library creates a thread for each request, for example sending 6 batch requests with each batch consisting of 500 device tokens. 6*500= 3k threads will be created. As I know, each thread requires 1 MB memory, this is another problem. Copied from: https://github.com/firebase/firebase-admin-java/issues/834#issuecomment-1890969169

jonathanedey commented 1 month ago

Hi @emindeniz99, Thanks for your patience on this!

Went ahead and mirrored most of those tests here.

The issue of threads is something we have been looking into along side http2. The main culprit being the default ThreadExecutor (although it reuses idle threads) has no thread limit which causes more harm than good. We're planning to address this in a separate PR.

Ideally we would want to make full use of Apache HttpClient 5's async clients and use CompletableFutures instead of a thread per request. However from my understanding this requires the google http client to also support async clients or an effort on our side to modify their classes to do so.

This transport in itself is a bandaid solution until async clients required for HTTP/2 are supported in google http client.

honorhs commented 1 month ago

Thank you for your efforts in providing the HTTP/2 functionality. It would be beneficial to have a report comparing the performance of the version with HTTP/2 applied and the previous version. This could include metrics such as system resources and processing completion time, under the assumption of sending a large number of push messages

diegomarquezp commented 3 weeks ago

This transport in itself is a bandaid solution until async clients required for HTTP/2 are supported in google http client.

cc: @burkedavison @ldetmer