Skip to content

Conversation

JamesNK
Copy link
Member

@JamesNK JamesNK commented May 1, 2023

#2002 added a host override to the DNS resolver. Usually this is good. However, in the container sample we want to access the original IP address.

@amcasey
Copy link
Contributor

amcasey commented May 1, 2023

Is this a pattern we expect actual users to (have to?) apply? Seems convoluted.

Edit: and now I see #2110 😆
Edit 2: That really only cleans up one line - I still don't really understand why it would be normal to know about and remove a bunch of overrides.

@JamesNK
Copy link
Member Author

JamesNK commented May 1, 2023

Users are unlikely ever to want to do this. The sample does this because it displays the host IP to make it clear that load balancing is happening.

@amcasey
Copy link
Contributor

amcasey commented May 1, 2023

Users are unlikely ever to want to do this. The sample does this because it displays the host IP to make it clear that load balancing is happening.

Maybe include a comment to that effect so people don't start copying it into their own apps without understanding?

@amcasey
Copy link
Contributor

amcasey commented May 1, 2023

Consume #2110?

@JamesNK
Copy link
Member Author

JamesNK commented May 1, 2023

The sample references a NuGet package. It needs to be released and published before the sample can use it. That is a couple of months away.

Copy link
Contributor

@amcasey amcasey left a comment

Choose a reason for hiding this comment

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

Seems reasonable.

{
public class ConfigurableResolverFactory : ResolverFactory
{
private static readonly BalancerAttributesKey<string> HostOverrideKey = new BalancerAttributesKey<string>("HostOverride");
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a little curious why we don't work directly with "HostOverride", rather than wrapping and unwrapping it, but I'm assuming this is idiomatic.

Copy link
Member Author

Choose a reason for hiding this comment

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

Its a pattern copied from the HttpRequestOptions collection. It's designed to make the collection feel a little more strongly typed.

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.

2 participants