-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
xds: Use XdsDependencyManager for XdsNameResolver #11957
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These replies are just for posterity. We discussed in-person.
Contributes to the gRFC A74 effort. https://github.com/grpc/proposal/blob/master/A74-xds-config-tears.md Co-authored-by: Eric Anderson <ejona@google.com>
The alternative to using Mockito's ArgumentMatcher is to use Hamcrest. However, Hamcrest did not impress me. ArgumentMatcher is trivial if you don't care about the error message.
This fixes a pre-existing issue where ConfigSelector.releaseCluster could revert the LB config back to using cluster manager after releasing all RPCs using a cluster have committed.
Unused since the context string was included directly into Data's StatusOr
I saw there's still a new TODO for a test, but I'll look at that later. |
} | ||
|
||
static final class StringContainsMatcher implements ArgumentMatcher<String> { | ||
private final String needle; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the variable names :)
I'm going to leave this in draft form until after I squash and rebase on master (once the review has gotten to that point).
"Gracefully handle tcp listeners. Fix error processing in XdsNR" is partly speculative right now, as we have a discussion of how error handling should be done. The tcp listeners is needed, but it got mixed together with onError() behavior. I can split it up needed.The gRFC changed to match the behavior here. grpc/proposal@fea3788