xds: add tests & misc fixes based on outstanding items #6935
Conversation
| @@ -84,7 +84,7 @@ public static XdsClientWrapperForServerSds newInstance( | |||
| Bootstrapper.BootstrapInfo bootstrapInfo = bootstrapper.readBootstrap(); | |||
| final List<Bootstrapper.ServerInfo> serverList = bootstrapInfo.getServers(); | |||
| if (serverList.isEmpty()) { | |||
| throw new NoSuchElementException("No management server provided by bootstrap"); | |||
| throw new FileNotFoundException("No management server provided by bootstrap"); | |||
There was a problem hiding this comment.
this does not look correct. it is not even an IO exception (this is why it is originally not an IO exception), consider using custom checked exception if there isn't any good exception to use.
There was a problem hiding this comment.
Yeah, I can create a custom checked exception or just return null with a log message. I think the latter is preferred but let me know your preference.
There was a problem hiding this comment.
it is definitely preferred to throw exception especially if the caller need to do something.
Looked at the code i think the getXdsClientWrapperForServerSds shouldn't return null. it should just throw and serverProtocolNegotiator can handle the exception (or just remove the getXdsClientWrapperForServerSds and inline it since it does not really belong to ServerSdsProtocolNegotiator); it sees exception so it can fallback seems more natural than hmm somehow we have null, lets fallback (not very idiomatic java, it is c style - no offense to c =p).
| @@ -60,7 +60,7 @@ private SdsProtocolNegotiators() { | |||
|
|
|||
| private static final Logger logger = Logger.getLogger(SdsProtocolNegotiators.class.getName()); | |||
|
|
|||
| private static final AsciiString SCHEME = AsciiString.of("https"); | |||
| private static final AsciiString SCHEME = AsciiString.of("http"); | |||
There was a problem hiding this comment.
IIRC there was a test checking scheme http if plaintext. the test should be fixed.
There was a problem hiding this comment.
There was/is no test - otherwise the build would have failed. Do you want me to add a test - the test will just verify this init so not sure about it's value.
There was a problem hiding this comment.
that test, it was checking "http" vs "https". now everything is "http". this test is no longer relevant and should be fixed.
There was a problem hiding this comment.
Yes, you are right, may bad! I will have to modify the test to detect plaintext vs Sds PN and let me think about it.
There was a problem hiding this comment.
With scheme being the same the only other thing to check is the class-name of the returned protocolNegotiator it should match ...ServerPlaintextNegotiator or should have plaintext in it. Do you have a better suggestion?
There was a problem hiding this comment.
yeah there isn't a good way to test this other than using name of the class. i think it is fine to check the class name (ServerPlaintextNegotiator).
|
thanks @creamsoup |
No description provided.