-
Notifications
You must be signed in to change notification settings - Fork 120
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
Discard not ready connections from HTTP/2 pool #227
base: main
Are you sure you want to change the base?
Conversation
HTTP2 connections will unregister themselves from the pool once enter connected_read_only and disconnected states, and register themselves once they reconnect. Fixes sneako#216
@@ -161,8 +161,6 @@ defmodule Finch.HTTP2.Pool do | |||
|
|||
@impl true | |||
def init({{scheme, host, port} = shp, registry, _pool_size, pool_opts}) do | |||
{:ok, _} = Registry.register(registry, shp, __MODULE__) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I remove the registering code here since we have another one after we enter connected state. IMO, registration happens after we establish the connection makes more sense
Thanks for taking this on! I am curious, did you run any kind of stress testing on this? I would like to see how the error rate and throughput compare to what we have in main. |
Oh, I haven't. Do we have any kind of stress testing/benchmark framework already in the repo? If not, do you have any suggestions about tools/frameworks? |
Sorry for the delay, but I finally got around to testing this out and found just a minimal performance regression but a total elimination of errors that are present on finch:main, so I think we should proceed with this change. I fixed up a couple things, but I want to merge #228 first, and then deal with any merge conflicts in this pr. |
@sneako is this the last issue for a new release? |
I made a application using OpenAI API via Finch HTTP2 pool. |
It could be. I would be very interested to know if this PR helps with the issue you are seeing. I have been struggling to find time to test this properly myself. |
HTTP2 connections will unregister themselves from the pool once enter connected_read_only and disconnected states, and register themselves once they reconnect.
Fixes #216