Skip to content

Remove host override from container sample#2109

Merged
JamesNK merged 2 commits into
grpc:masterfrom
JamesNK:jamesnk/container-removehostoverride
May 2, 2023
Merged

Remove host override from container sample#2109
JamesNK merged 2 commits into
grpc:masterfrom
JamesNK:jamesnk/container-removehostoverride

Conversation

@JamesNK

@JamesNK JamesNK commented May 1, 2023

Copy link
Copy Markdown
Member

#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

amcasey commented May 1, 2023

Copy link
Copy Markdown
Contributor

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

JamesNK commented May 1, 2023

Copy link
Copy Markdown
Member Author

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

amcasey commented May 1, 2023

Copy link
Copy Markdown
Contributor

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

amcasey commented May 1, 2023

Copy link
Copy Markdown
Contributor

Consume #2110?

@JamesNK

JamesNK commented May 1, 2023

Copy link
Copy Markdown
Member Author

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.

Comment thread examples/Container/Frontend/Balancer/ConfigurableResolverFactory.cs

@amcasey amcasey left a comment

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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
Copy Markdown
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