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.
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
internal/resolver: introduce a new delegating resolver to handle both target URI and proxy address resolution #7857
internal/resolver: introduce a new delegating resolver to handle both target URI and proxy address resolution #7857
Changes from 13 commits
bccef6b
6b89e0d
7c2fb21
e8bb6bc
36d0573
5a87284
e05a895
e724a9a
975e235
4b8ade9
4ba3578
fa55e5b
df11d7c
8bcf2b6
a5aeb9a
47e7936
a2e1415
df87a68
ed3d577
dd5bdb3
d24ec8b
74c6cd0
31d8c2c
1643768
4a5ab5b
fd061c7
c6dbcbc
2be9db3
67e3799
f5e1d37
019f621
8348b1f
f0d5b7b
4ac31f9
ed413f6
402316c
dea9ddd
40f1035
95d98b9
d5f4d8f
e743cbf
e008feb
4aea07d
ed345a1
7f416b0
e68d5de
8848105
61edca1
a01cc7a
845680d
c752bd8
d53100c
99292ea
74b3cf6
c745144
628a2a4
eeae936
bc0adeb
9c2ce28
4482e51
2bafe77
a0a2e50
ca2cc24
1f1e7f5
402badf
163ad20
db8b92c
6623eb5
c4bde98
1c4b416
5dc893c
69253c7
c5f8d6f
bbea8c4
fc2c41f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Check warning on line 93 in internal/envconfig/envconfig.go
Codecov / codecov/patch
internal/envconfig/envconfig.go#L93
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.
The three bullet points here are more relevant to code in the channel where it is deliberating whether to create a delegating resolver or not. This is not useful here, especially in the package level docstring.
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.
Removed.
Check warning on line 68 in internal/resolver/delegatingresolver/delegatingresolver.go
Codecov / codecov/patch
internal/resolver/delegatingresolver/delegatingresolver.go#L67-L68
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.
If you are planning on using the
delegating_resolver
prefix for errors returned from this package, please be consistent and do it at all places.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.
Done.
Check warning on line 84 in internal/resolver/delegatingresolver/delegatingresolver.go
Codecov / codecov/patch
internal/resolver/delegatingresolver/delegatingresolver.go#L83-L84
Check warning on line 89 in internal/resolver/delegatingresolver/delegatingresolver.go
Codecov / codecov/patch
internal/resolver/delegatingresolver/delegatingresolver.go#L88-L89
Check warning on line 102 in internal/resolver/delegatingresolver/delegatingresolver.go
Codecov / codecov/patch
internal/resolver/delegatingresolver/delegatingresolver.go#L101-L102
Check warning on line 107 in internal/resolver/delegatingresolver/delegatingresolver.go
Codecov / codecov/patch
internal/resolver/delegatingresolver/delegatingresolver.go#L106-L107
Check warning on line 115 in internal/resolver/delegatingresolver/delegatingresolver.go
Codecov / codecov/patch
internal/resolver/delegatingresolver/delegatingresolver.go#L114-L115
Check warning on line 120 in internal/resolver/delegatingresolver/delegatingresolver.go
Codecov / codecov/patch
internal/resolver/delegatingresolver/delegatingresolver.go#L119-L120
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.
Consider implementing a
nopResolver
instead that just ignores all calls, so you don't need to spread!= nil
checks throughout the code (and get a panic if you forget one).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.
But since delegating resolver is the default resolver in most cases, doesn't it make sense that if ClientConn calls
ResolveNow
orClose
, we propagate the same to the child resolvers?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.
What I meant:
Then you never need to check if these fields are nil, since they are always initialized.
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.
Done.
Check warning on line 132 in internal/resolver/delegatingresolver/delegatingresolver.go
Codecov / codecov/patch
internal/resolver/delegatingresolver/delegatingresolver.go#L126-L132
Check warning on line 141 in internal/resolver/delegatingresolver/delegatingresolver.go
Codecov / codecov/patch
internal/resolver/delegatingresolver/delegatingresolver.go#L135-L141
Check warning on line 161 in internal/resolver/delegatingresolver/delegatingresolver.go
Codecov / codecov/patch
internal/resolver/delegatingresolver/delegatingresolver.go#L159-L161
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.
Please remove this blank line
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.
Done
Check warning on line 168 in internal/resolver/delegatingresolver/delegatingresolver.go
Codecov / codecov/patch
internal/resolver/delegatingresolver/delegatingresolver.go#L166-L168
Check warning on line 175 in internal/resolver/delegatingresolver/delegatingresolver.go
Codecov / codecov/patch
internal/resolver/delegatingresolver/delegatingresolver.go#L173-L175
Check warning on line 188 in internal/resolver/delegatingresolver/delegatingresolver.go
Codecov / codecov/patch
internal/resolver/delegatingresolver/delegatingresolver.go#L187-L188
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.
Let's figure out if we can get rid of this. Definitely a string should not be used. An enum (enum-lite, since this is Go) would be better. Can we maybe store a pointer to the appropriate one of
parent.targetAddrs
orparent.proxyAddrs
instead?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 have changed it to use the enum but not the pointer to the address as we will need to know which resolver is the update from to pass the service config of the target resolver to the LB policy.
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.
Isn't it a bug to not do this unconditionally?
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 have not done it unconditionally because I do that so that the service config and the other state information can be passed and becomes the part of curState.
I am not sure as to why will it be a bug because this
UpdateState
is only called in case of proxy and not return state unless both resolvers send their update.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.
A comment to explain why
curState
is updated only for updates from the target resolver would help.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.
Done.
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.
This cannot completely eat
UpdateState
calls. If a channel has zero addresses being reported by a resolver, that is an error, and the resolver needs to receive an error in response.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 dont see an error being returned by
cc.UpdateState
currently for zero addresses. As of today, the balancer returns an error forresolver produced zero addresses
which should work for the delegating resolver as well.I used this condition to check if both the proxy and the target resolver have sent the update. I missed the case that the resolvers could send 0 addresses as well and it might not work correctly.
I am changing the condition to check the updates from both resolvers and letting the balancer handle the zero address error, because if either of the resolvers send zero address, the balancer will get zero address and act accordingyly.
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.
Please fix grammar in this comment.
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.
Done.
Check warning on line 224 in internal/resolver/delegatingresolver/delegatingresolver.go
Codecov / codecov/patch
internal/resolver/delegatingresolver/delegatingresolver.go#L223-L224
Check warning on line 230 in internal/resolver/delegatingresolver/delegatingresolver.go
Codecov / codecov/patch
internal/resolver/delegatingresolver/delegatingresolver.go#L229-L230
Check warning on line 236 in internal/resolver/delegatingresolver/delegatingresolver.go
Codecov / codecov/patch
internal/resolver/delegatingresolver/delegatingresolver.go#L235-L236