-
Notifications
You must be signed in to change notification settings - Fork 24
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
base: main
Are you sure you want to change the base?
Conversation
uris2.lock().unwrap().push(endpoint.uri().clone()); | ||
Some(endpoint) | ||
}) | ||
.with_endpoint_layer(|endpoint: Endpoint| endpoint.user_agent("my ginepro client").ok()) |
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.
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?
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.
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
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.
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
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 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.
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.
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
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.
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?
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.
@tl-helge-hoff are you asking for eagerly evaluated Endpoint
s or am I misinterpreting?
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. |
Addresses #31. Adds a new
EndpointMiddleware
trait and adds thewith_endpoint_layer
method to theLoadBalancedChannelBuilder
. These middlewares are invoked on everySocketAddr
that the lookup service discovers.Usage: