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

fix: use sendThreadPool in sendEvents #107

Merged
merged 5 commits into from
Oct 10, 2024

Conversation

kevink-sq
Copy link
Contributor

@kevink-sq kevink-sq commented Aug 27, 2024

Summary

Issue #108

This PR consists of 2 main changes to address a performance bottleneck in the Amplitude client:
(1) Pass cached thread pool to supplyAsync
(2) Allow users to set sendThreadPool and retryThreadPool

During performance testing it was found Amplitude client would bottleneck on the default ForkJoinPool invoked indirectly via CompletableFuture.supplyAsync(runnable) which is not ideal for I/O:

Screenshot 2024-08-27 at 4 06 44 PM

Many threads were parked due to Amplitude during testing and increasing the fork join pool resolved the issue.

Checklist

  • Does your PR title have the correct title format?
  • Does your PR have a breaking change?: no

@kevink-sq kevink-sq force-pushed the fix-http-transport-send-thread-pool branch from 8191770 to dd71826 Compare August 28, 2024 04:10
@kevink-sq kevink-sq force-pushed the fix-http-transport-send-thread-pool branch from dd71826 to 9069886 Compare August 28, 2024 04:15
@kevink-sq kevink-sq force-pushed the fix-http-transport-send-thread-pool branch from a081734 to 5b45326 Compare September 19, 2024 23:34
@izaaz
Copy link
Contributor

izaaz commented Sep 26, 2024

@kevink-sq - It looks like one of the tests is failing with this change. Since we are using the same thread pool for both sending with retry and sending the actual events, the task to send with retry uses up a few threads on the pool and then chokes the actual sending tasks? This problem goes away when you use two different thread pools for send with retry and actual sending or if you replace the executor service with a fork join pool.

@kevink-sq
Copy link
Contributor Author

@izaaz - Updated the PR to run supplyAsync in the cached threadpool (which has no bound). You were right about the reused sendThreadPool locking up tasks and I'm glad the tests caught that. Please let me know if you require anything else.

@kevink-sq
Copy link
Contributor Author

@izaaz bump

@kevink-sq
Copy link
Contributor Author

@izaaz gentle bump

@izaaz izaaz merged commit 88a9589 into amplitude:main Oct 10, 2024
5 checks passed
justin-fiedler pushed a commit that referenced this pull request Oct 10, 2024
## [1.12.4](v1.12.3...v1.12.4) (2024-10-10)

### Bug Fixes

* use sendThreadPool in sendEvents ([#107](#107)) ([88a9589](88a9589))
Copy link

🎉 This PR is included in version 1.12.4 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants