xds: listener type validation #11933
Conversation
shivaspeaks
added
the
kokoro:run
grpc-kokoro
removed
the
kokoro:run
| @Override | ||
| public void onResourceDoesNotExist(final String resourceName) { | ||
| if (stopped) { | ||
| return; | ||
| } | ||
| StatusException statusException = Status.UNAVAILABLE.withDescription( | ||
| String.format("Listener %s unavailable, xDS node ID: %s", resourceName, | ||
| String.format("%s listener unavailable, xDS node ID: %s", resourceName, |
There was a problem hiding this comment.
nit: what would be the reason to switch the order of the error format here? I think Listener %s is slightly more common in the code base. Consistent formatting helps with searching when debugging issues.
|
While it is in the gRFC, why is it really important to enforce a returned address being non-null? This doesn't seem to really provide value as the address was already known for communicating to the xds server in the first place. |
The returned Listener address tells the server what to listen on. We didn't remove it. We just made it so that the control plane can have a trivial job of filling in the correct value. |
| public void onChanged_listenerAddressMismatch() | ||
| throws ExecutionException, InterruptedException, TimeoutException { | ||
|
|
||
| when(mockBuilder.build()).thenReturn(mockServer); |
There was a problem hiding this comment.
This is already done in @Before. Delete it?
| filterRegistry, executor.getScheduledExecutorService()); | ||
|
|
||
| final SettableFuture<Server> start = SettableFuture.create(); | ||
| Executors.newSingleThreadExecutor().execute(new Runnable() { |
There was a problem hiding this comment.
This thread is going to be hanging and outlive the test. Run xdsServerWrapper.shutdownNow() at the end to let the start() return. Ideally you'd wait for the start future to complete as well (which will throw an exception), so you know the thread has (essentially) returned.
There was a problem hiding this comment.
There is a teardown happening after every test in @After which does xdsServerWrapper.shutdownNow(). So I doubt if we should be adding in the test as well!
There was a problem hiding this comment.
Well, shutdownNow can be called twice. I'm not wild about the lifecycles in the test as we won't notice if it is broken. But I think a lot of these tests are bad already, so let's call it good enough as-is.
| filterRegistry, executor.getScheduledExecutorService()); | ||
|
|
||
| final SettableFuture<Server> start = SettableFuture.create(); | ||
| Executors.newSingleThreadExecutor().execute(new Runnable() { |
There was a problem hiding this comment.
Well, shutdownNow can be called twice. I'm not wild about the lifecycles in the test as we won't notice if it is broken. But I think a lot of these tests are bad already, so let's call it good enough as-is.
Fixes #11737