Xds client split #11484
Xds client split #11484
Conversation
| @@ -51,6 +51,7 @@ public class SharedXdsClientPoolProviderTest { | |||
| @Rule | |||
| public final ExpectedException thrown = ExpectedException.none(); | |||
| private final Node node = Node.newBuilder().setId("SharedXdsClientPoolProviderTest").build(); | |||
| private static final String DUMMY_TARGET = "dummy"; | |||
There was a problem hiding this comment.
We should test at least two targets, and verify, e.g. the second xdsClient is not the same instance as the first one, is ref counted.
There was a problem hiding this comment.
I see that multiple targets have been covered in csds service test. Still, it would be nice to have multiple targets tested in this UT.
| String resourceType = value.getTypeUrl(); | ||
| if (!value.getResponseNonce().isEmpty() | ||
| && !String.valueOf(xdsNonces.get(resourceType)).equals(value.getResponseNonce())) { | ||
| if (!value.getResponseNonce().isEmpty() && xdsNonces.containsKey(resourceType) |
There was a problem hiding this comment.
this change also seems not related?
There was a problem hiding this comment.
This fix is needed to support anything trying a nonce other than the initial empty value.
There was a problem hiding this comment.
I still don't see this change related to "support multiple xds client". Looks this file can be reverted without impacting anything?
But I see this potentially avoid NPE when it receives another resource type other than the hardcoded resource type in xdsNonces keyset.
| @@ -51,6 +51,7 @@ public class SharedXdsClientPoolProviderTest { | |||
| @Rule | |||
| public final ExpectedException thrown = ExpectedException.none(); | |||
| private final Node node = Node.newBuilder().setId("SharedXdsClientPoolProviderTest").build(); | |||
| private static final String DUMMY_TARGET = "dummy"; | |||
There was a problem hiding this comment.
I see that multiple targets have been covered in csds service test. Still, it would be nice to have multiple targets tested in this UT.
| TimeProvider.SYSTEM_TIME_PROVIDER, | ||
| MessagePrinter.INSTANCE, | ||
| new TlsContextManagerImpl(bootstrapInfo)); | ||
| } catch (Exception e) { |
There was a problem hiding this comment.
What is this catching? I don't see any new exceptions in #11254 either.
|
You don't need to wait for me to merge this. I wasn't planning on providing a full review. I just poked in and had the drive-by comment. |
Split off from #11254