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

core,grpclb: DnsNameResolver will use srv record by default if grpclb #6298

Merged
merged 4 commits into from Oct 18, 2019

Conversation

creamsoup
Copy link
Contributor

@creamsoup creamsoup commented Oct 17, 2019

The DNS name resolver will look up SRV records by default if the grpclb artifact is in the classpath.


@Override
protected boolean isSrvEnabled() {
return Boolean.parseBoolean(System.getProperty(ENABLE_GRPCLB_PROPERTY_NAME, "true"));
Copy link
Contributor

Choose a reason for hiding this comment

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

According to the design, GRPCLB will have SRV enabled "unconditionally". Just return "true" here, then the test can be largely simplified.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i discussed with @ejona86, we decided to make that conditional for debugging purpose etc.
i made a local change to make it testable, but it is pretty nasty (using class loader and reflection to reevaluate class... not many lines of code but yup....) i am not sure we want to use this approach or not.


@RunWith(JUnit4.class)
public class GrpclbNameResolverProviderTest {

Copy link
Contributor

Choose a reason for hiding this comment

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

We should verify that this provider's priority is higher than the core provider's.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

without using reflection it is not possible to test since the priority is protected and NameResolverProvider is hiding everything.

@creamsoup
Copy link
Contributor Author

added tests for default value, PTAL

@creamsoup creamsoup merged commit eef47b2 into grpc:master Oct 18, 2019
@creamsoup creamsoup deleted the srv branch October 18, 2019 20:00
creamsoup added a commit to creamsoup/grpc-java that referenced this pull request Oct 18, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Jan 16, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants