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

Discard not ready connections from HTTP/2 pool #227

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Goose97
Copy link

@Goose97 Goose97 commented May 25, 2023

HTTP2 connections will unregister themselves from the pool once enter connected_read_only and disconnected states, and register themselves once they reconnect.

Fixes #216

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__)
Copy link
Author

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

@sneako
Copy link
Owner

sneako commented May 25, 2023

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.

@Goose97
Copy link
Author

Goose97 commented May 25, 2023

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?

@zachallaun zachallaun mentioned this pull request Jun 11, 2023
10 tasks
@sneako
Copy link
Owner

sneako commented Jun 11, 2023

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.

@hkrutzer
Copy link

@sneako is this the last issue for a new release?

@nallwhy
Copy link

nallwhy commented Dec 30, 2023

I made a application using OpenAI API via Finch HTTP2 pool.
And I got so much (Finch.Error) disconnected errors.
Is this issue related to the problem?

@sneako
Copy link
Owner

sneako commented Dec 30, 2023

I made a application using OpenAI API via Finch HTTP2 pool.

And I got so much (Finch.Error) disconnected errors.

Is this issue related to the problem?

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.

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

Successfully merging this pull request may close these issues.

Discussion: should HTTP/2 pool skip over not ready connections?
4 participants