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

add endpoint middlewares #34

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

Conversation

conradludgate
Copy link
Contributor

@conradludgate conradludgate commented May 18, 2022

Addresses #31. Adds a new EndpointMiddleware trait and adds the with_endpoint_layer method to the LoadBalancedChannelBuilder. These middlewares are invoked on every SocketAddr that the lookup service discovers.

Usage:

let load_balanced_channel = LoadBalancedChannel::builder(("www.test.com", 5000))
        // set the concurrency limit for the endpoint
        .with_endpoint_layer(|endpoint: Endpoint| Some(endpoint.concurrency_limit(1)))
        // set the user agent for the endpoint
        .with_endpoint_layer(|endpoint: Endpoint| {
            endpoint.user_agent("my ginepro client").ok()
        })
        .channel()
        .await
        .unwrap();

@conradludgate conradludgate requested a review from a team as a code owner May 18, 2022 10:56
uris2.lock().unwrap().push(endpoint.uri().clone());
Some(endpoint)
})
.with_endpoint_layer(|endpoint: Endpoint| endpoint.user_agent("my ginepro client").ok())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couldn't this cause a silent bug in the sense that the function fails but you still build the channel and your configuration is not applied as a consequence. My question is: do we want to the callback to return the error so the internals can fail the operation?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does! The function returns an Option<Endpoint>, returning None will short circuit, and if it can't build the endpoint, it won't be added to list of endpoints

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This functions the same as if you got your TLS config wrong. That does report a warning though. I'll see if I can make these convenient fn impls auto emit warnings

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I phrased my question wrong. Ideally these configurations should be verified before the builder succeeds, but that's impossible to do give the nature of the api and the protocol. Just wanted to clarify whether we're happy with that behavior: if you try to configure the endpoints but it consistently fails at runtime (for example tls_config or user_agent fails) no endpoints would be included. But as I'm writing this I don't see how we would handle it differently.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah the awkward thing is that this is only the probe service which runs in a background task. If it would error and close the probe loop, it would not effect the channel in any way.

I wonder if we can force the balanced channel to close in the event that the probe task closes

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about making a wrapper type? only exposing the infallible methods? because Endpoint also has connect which we don't want anyone to call, and the tls config we already do internally in the lib. WDYT?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tl-helge-hoff are you asking for eagerly evaluated Endpoints or am I misinterpreting?

@jspeiser
Copy link

jspeiser commented May 26, 2024

Is there any chance of this PR being merged? I've been investigating issues with very low-throughput connections and believe configuring endpoint settings might help.

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

Successfully merging this pull request may close these issues.

None yet

4 participants