Merged
Conversation
markdroth
force-pushed
the
c++_resolver
branch
from
b95583c to
46dd789
Compare
This was referenced Dec 21, 2017
Merged
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
Contributor
|
Reviewed 1 of 48 files at r1, 48 of 51 files at r6, 1 of 1 files at r7, 2 of 2 files at r8. src/core/ext/filters/client_channel/resolver_registry.h, line 76 at r8 (raw file):
Suggest calling this src/core/ext/filters/client_channel/resolver/dns/c_ares/dns_resolver_ares.cc, line 87 at r8 (raw file):
Nit: s/Closures/closures/ src/core/ext/filters/client_channel/resolver/dns/c_ares/dns_resolver_ares.cc, line 184 at r8 (raw file):
We may want to reset Comments from Reviewable |
AspirinSJL
approved these changes
Jan 12, 2018
Member
Author
|
Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed. src/core/ext/filters/client_channel/resolver_registry.h, line 76 at r8 (raw file): Previously, AspirinSJL (Juanli Shen) wrote…
Done. src/core/ext/filters/client_channel/resolver/dns/c_ares/dns_resolver_ares.cc, line 87 at r8 (raw file): Previously, AspirinSJL (Juanli Shen) wrote…
Done. src/core/ext/filters/client_channel/resolver/dns/c_ares/dns_resolver_ares.cc, line 184 at r8 (raw file): Previously, AspirinSJL (Juanli Shen) wrote…
This isn't related to the rest of this PR, so I'd prefer not to change it here. Comments from Reviewable |
Member
Author
|
Note: This is now based on #13984. |
|
|
|
|
|
|
|
|
|
|
|
Contributor
|
Reviewed 51 of 51 files at r10. src/core/ext/filters/client_channel/client_channel.cc, line 571 at r10 (raw file):
nit: the src/core/ext/filters/client_channel/resolver.h, line 54 at r10 (raw file):
s/NULL/nullptr src/core/ext/filters/client_channel/resolver.h, line 72 at r10 (raw file):
We may want to mention src/core/ext/filters/client_channel/resolver_registry.cc, line 57 at r10 (raw file):
why does this need to modify src/core/ext/filters/client_channel/resolver_registry.cc, line 80 at r10 (raw file):
the magic number 10 should be behind a named constexpr variable to document its meaning. Comments from Reviewable |
Contributor
|
Looks great. Just minor comments/suggestions. Review status: all files reviewed at latest revision, 5 unresolved discussions. Comments from Reviewable |
Member
Author
|
Review status: all files reviewed at latest revision, 5 unresolved discussions. src/core/ext/filters/client_channel/client_channel.cc, line 571 at r10 (raw file): Previously, dgquintas (David G. Quintas) wrote…
Done. src/core/ext/filters/client_channel/resolver.h, line 54 at r10 (raw file): Previously, dgquintas (David G. Quintas) wrote…
Done. src/core/ext/filters/client_channel/resolver.h, line 72 at r10 (raw file): Previously, dgquintas (David G. Quintas) wrote…
That arg is specific to the DNS resolver, not part of the generic resolver API, so I don't think it's appropriate to mention it here. src/core/ext/filters/client_channel/resolver_registry.cc, line 57 at r10 (raw file): Previously, dgquintas (David G. Quintas) wrote…
Added a comment explaining the API here. I agree that it's a little messy, but it's only used internally to this file, so I think it's fine. src/core/ext/filters/client_channel/resolver_registry.cc, line 80 at r10 (raw file): Previously, dgquintas (David G. Quintas) wrote…
I think it's very unlikely that this will ever need to be changed. I've added a comment about it. Comments from Reviewable |
|
|
|
dgquintas
approved these changes
Feb 8, 2018
markdroth
force-pushed
the
c++_resolver
branch
from
22efeca to
209f644
Compare
vjpai
approved these changes
Feb 8, 2018
Contributor
vjpai
left a comment
vjpai
left a comment
There was a problem hiding this comment.
Approval on changes in test/cpp/end2end . No API concerns
|
|
|
Member
Author
|
Green! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is built on #13659 and #13676 (both of which have now been merged).
This change is