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.

Sign up for GitHub

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

Jump to bottom

Add support for service configs to c-ares resolver. #11954

Merged
merged 1 commit into from Aug 3, 2017
Merged

Add support for service configs to c-ares resolver. #11954

merged 1 commit into from Aug 3, 2017

Conversation

markdroth
Copy link
Member

This implements the functionality described in gRFC A2:

https://github.com/grpc/proposal/blob/master/A2-service-configs-in-dns.md

This also fixes a bug in the code from #10706, which broke the ability to actually specify an alternate DNS server.

I've done some basic manual testing of this, but there are no automated tests yet. In order to do that, I think we'll need to figure out how to run a DNS server in the automated test framework.

@markdroth markdroth requested a review from y-zeng July 27, 2017 16:42
@grpc-kokoro
Copy link 

[microbenchmarks] No significant performance differences

@grpc-kokoro
Copy link 

Corrupt JSON data (indicates timeout or crash): 
    bm_fullstack_trickle.BM_PumpStreamServerToClient_Trickle_1_4k.opt.new: 1


[trickle] No significant performance differences

y-zeng
y-zeng reviewed Jul 27, 2017
View reviewed changes
src/core/ext/filters/client_channel/resolver/dns/c_ares/dns_resolver_ares.c
}
grpc_json *service_config_json = NULL;
for (grpc_json *field = choice->child; field != NULL; field = field->next) {
// Check client language, if specified.
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we still use C style comments?

Copy link
Member Author

Choose a reason for hiding this comment

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

C++-style comments are fine. They're legal in C99.

src/core/ext/filters/client_channel/resolver/dns/c_ares/dns_resolver_ares.c Outdated
grpc_lb_addresses_create_channel_arg(r->lb_addresses);
grpc_service_config *service_config = NULL;
if (r->service_config_json != NULL) {
char* service_config_string =
Copy link
Contributor

Choose a reason for hiding this comment

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

We may need to free service_config_string after grpc_channel_args_copy_and_add_and_remove() is called.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch! Fixed.

@y-zeng
Copy link
Contributor

y-zeng commented Jul 27, 2017

This looks great! Please also update the tests that are using grpc_dns_lookup_ares().

@grpc-kokoro
Copy link 

[trickle] No significant performance differences

@grpc-kokoro
Copy link 

[microbenchmarks] No significant performance differences

y-zeng
y-zeng reviewed Jul 28, 2017
View reviewed changes
Copy link
Contributor

@y-zeng y-zeng left a comment

Choose a reason for hiding this comment

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

Please also update grpc_ares_wrapper_fallback.c.

src/core/ext/filters/client_channel/resolver/dns/c_ares/dns_resolver_ares.c Outdated
}
// Check client hostname, if specified.
if (strcmp(field->key, "clientHostname") == 0) {
char hostname[HOST_NAME_MAX];
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like we don't have HOST_NAME_MAX on mac. It's recommended by posix, but not required to be defined.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup, was already working on that.

Done.

@markdroth
Copy link
Member Author

@nicolasnoble, please take a look at b826aa7 and let me know if I'm handling the portability issues the right way. Thanks!

@grpc-kokoro
Copy link 

[trickle] No significant performance differences

1 similar comment
@grpc-kokoro
Copy link 

[trickle] No significant performance differences

@grpc-kokoro
Copy link 

[microbenchmarks] No significant performance differences

nicolasnoble
nicolasnoble reviewed Aug 1, 2017
View reviewed changes
src/core/lib/iomgr/gethostname_fallback.c Outdated

#include "src/core/lib/iomgr/port.h"

#if !defined(GRPC_POSIX_HOST_NAME_MAX) && !defined(GRPC_POSIX_SYSCONF)
Copy link
Member

Choose a reason for hiding this comment

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

The only small maintainability problem I see here potentially is if there's yet a new version of grpc_gethostname that gets added, this logic here is disjointed from adding a new entry (which resides in port.h).

Our main way to handle this is to add for instance GRPC_GETHOSTNAME_FALLBACK in port.h close to the other checks where people would need to add their new implementation anyway, for example where you check for duplicated #defines, you can also add the fallback definition.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@grpc-kokoro
Copy link 

[trickle] No significant performance differences

@grpc-kokoro
Copy link 

[microbenchmarks] No significant performance differences

@grpc-kokoro
Copy link 

[trickle] No significant performance differences

@grpc-kokoro
Copy link 

[microbenchmarks] No significant performance differences

@markdroth markdroth merged commit dca911e into grpc:master Aug 3, 2017
@markdroth markdroth deleted the service_config_dns branch August 3, 2017 21:48
@lock lock bot locked as resolved and limited conversation to collaborators Jan 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@dgquintas dgquintas dgquintas approved these changes

@nicolasnoble nicolasnoble nicolasnoble approved these changes

@y-zeng y-zeng y-zeng approved these changes

@ctiller ctiller Awaiting requested review from ctiller

Assignees

@y-zeng y-zeng

Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@markdroth @grpc-kokoro @y-zeng @dgquintas @nicolasnoble @googlebot