xds: a myriad of cleanups #4479
Conversation
dfawley
changed the title
dfawley
force-pushed
the
cleanups
branch
2 times, most recently
from
21296fb
to
d6f1d66
Compare
|
I love this title. |
| @@ -408,9 +409,6 @@ func (b *cdsBalancer) run() { | |||
| b.edsLB.Close() | |||
| b.edsLB = nil | |||
| } | |||
| if newXDSClient != nil { | |||
There was a problem hiding this comment.
Is this an intended change, or merge conflict?
There was a problem hiding this comment.
I think I know why this change is here.
I would say don't make this change, or at least, don't mix it in this XL PR.
And it's an incomplete test cleanup. It leaves the balancers in an inconsistent state (if the balancer creates the client, it should call close, even though we know close is a noop)
And it only changes the easy part, but leaves the hard part behind. We should modify the tests to use attributes to pass the fake xDS client, so we can completely get rid of newXDSClient.
But that's going to be a semi big change, it requires changing signature of xds.SetClient() to take an interface instead of *Client, so it can be the fakeClient.
I will make it, I need it to fix the tests in my other PR.
There was a problem hiding this comment.
And it's an incomplete test cleanup. It leaves the balancers in an inconsistent state (if the balancer creates the client, it should call close, even though we know close is a noop)
It's for tests only, so I don't think it's important to call Close only for tests, which don't need it to be called. Many types don't need a Close method to be called when they're done being used. Our xdsClient interface is one of those.
You could also consider this half of the cleanup (removing the method from the interface and the call to it in b.Close), with the other half being changing how it's injected. I can add all this back if you want, but I do see it as a step in the right direction.
There was a problem hiding this comment.
I still think this doesn't give us enough benefit, to justify itself.
At least should not be mixed in a 84-file (trivial) change PR.
This diff really stick out when I was scanning the changes.
| x.edsImpl.updateState(u.priority, u.s) | ||
| case <-x.closed.Done(): | ||
| x.cancelWatch() | ||
| if newXDSClient != nil { |
| @@ -256,7 +255,4 @@ func (w *xdsClientWrapper) close() { | |||
| w.cancelLoadReport() | |||
| w.cancelLoadReport = nil | |||
| } | |||
| if newXDSClient != nil { | |||
| w.c.Close() | |||
|
|
||
| // WaitForClose waits for Close to be invoked on this client and returns | ||
| // context.DeadlineExceeded otherwise. | ||
| func (xdsC *Client) WaitForClose(ctx context.Context) error { |
There was a problem hiding this comment.
This isn't right.
The actual question is, why is this method not used in any tests (it should be all the tests, but not only the resolver tests would matter).
The right fix is to call and check for this in the xds resolver tests, not to delete this.
There was a problem hiding this comment.
The actual question is, why is this method not used in any tests (it should be all the tests, but not only the resolver tests would matter).
I had assumed this was once here for some kind of synchronization, then the usage was removed because it wasn't necessary.
This doesn't seem like the right way to test that the resolver closes the client. We should be ensuring that by the time xdsResolver.Close() returns, xdsClient.Close() has been called -- not "block until it happens".
I can add this test case in another PR immediately after this if you want.
There was a problem hiding this comment.
Same as the other comment. This change should be moved to the other PR.
There was a problem hiding this comment.
I'll just close this PR and open a series of other PRs, one for each change.
| } | ||
| if newXDSClient != nil { |
There was a problem hiding this comment.
This is same as the others.
The first commit is branched from #4476
mv xds/internal/client xds/internal/xdsclientxdsclient; those that didn't were confusing. Some double imported because of this (cdsbalancer_test.goandeds_impl_test.go, maybe others).Interfacefrom the names of some interfaces. Types aren't supposed to be named by their types, generally.bxdsClient(was:xdsCorclientin some)RELEASE NOTES: none