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

Jettison generic pool from outbound #3115

Merged
merged 7 commits into from
Dec 17, 2022
Merged

Jettison generic pool from outbound #3115

merged 7 commits into from
Dec 17, 2022

Conversation

msimerson
Copy link
Member

@msimerson msimerson commented Dec 2, 2022

Related to #3100
Fixes #2717

Checklist:

  • docs updated
  • tests updated
  • Changes updated

Background

The use of generic-pool has long been problematic. After a long period of being stuck on v2, I recently updated to v3, which has a new promises API. That update then started causing problems for some users because generic-pool started emitting new timeout errors, which weren't mentioned in the updating docs. Hours of tracing and debugging. I also discovered that a function we used was also removed entirely and again, no mention in the docs, so I also fixed that. So now our pooling code is back to working as well as it ever has, which is to say, it mostly works, but it still has a number of edge cases, a few of which are documented in #2717, #2945, #3022, and #3100.

Part of me thinks, "disable pooling code by default and document it with a THAR BE DRAGONS." The rest of me thinks, it's code that introduces a lot of complexity while chasing after a little bit of performance, get rid of it.

@gramakri
Copy link
Collaborator

gramakri commented Dec 5, 2022

Thanks @msimerson , I will test this out later today/tomorrow.

@msimerson msimerson merged commit 3115810 into master Dec 17, 2022
@msimerson msimerson deleted the jettison-generic-pool branch December 17, 2022 23:34
@gramakri
Copy link
Collaborator

gramakri commented Dec 28, 2022

Sorry, first the internet was down and then I got the flu last few weeks.

@msimerson I tested this now and all our CI tests pass. I will report any issues as I find them. Thanks!

@gramakri
Copy link
Collaborator

gramakri commented Jan 2, 2023

No issues so far.

@msimerson
Copy link
Member Author

Same here. I was able to do additional testing and deployments before the holiday week and all is looking good.

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

Successfully merging this pull request may close these issues.

[Outbound] Ongoing connection times out at connect_timeout instead of pool_timeout
2 participants