FrankChen021 commented on code in PR #20688:
URL: https://github.com/apache/druid/pull/20688#discussion_r4348938788
##########
processing/src/main/java/org/apache/druid/java/util/http/client/pool/ChannelResourceFactory.java:
##########
@@ -89,7 +90,7 @@ public ChannelFuture generate(final String hostname)
log.debug("Generating: %s", hostname);
URL url;
try {
- url = new URL(hostname);
+ url = URI.create(hostname).toURL();
}
catch (MalformedURLException e) {
throw new RuntimeException(e);
Review Comment:
I don't know why the `toURL` is design to throw two exceptions, based on our
change, catching two exceptions makes the code complicated and it's very easy
to forget to catch `IllegalArgumentException` and people may ask why such
exception is not thrown (for example here) but thrown in other places.
So it requires a case-by-case analysis for example there, the
`IllegalArgumentException` is type of RuntimeException exception so directly
thrown here matches original behaviour.
So I think it would be safer for us to define a public static util method to
replace current `new URL` as below :
```java
static URL toURL(final String value) throws MalformedURLException
{
try {
return URI.create(value).toURL();
}
catch (IllegalArgumentException e) {
final MalformedURLException exception =
new MalformedURLException("Invalid URL [" + value + "]");
exception.initCause(e);
throw exception;
}
}
```
and when adding forbidden rule in future, we can say that above toURL should
be used.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
