Java: remove URL #41
Java: remove URL #41
Conversation
| return new URL(alternatorScheme, nextNode(), alternatorPort, file); | ||
| } catch (MalformedURLException e) { | ||
| return new URI(alternatorScheme, "", nextNode(), alternatorPort, file, "", ""); | ||
| } catch (URISyntaxException e) { | ||
| // Can only happen if alternatorScheme is an unknown one. | ||
| logger.log(Level.WARNING, "nextAsURL", e); |
There was a problem hiding this comment.
This part and comment should be updated too. URI won't throw if provided with unknown to URL format scheme.
There was a problem hiding this comment.
Thanks, removed the comment.
There was a problem hiding this comment.
nit: nextAsURL inside logger.log( should change to nextAsURI
| try { | ||
| // Note that despite this being called HttpURLConnection, it actually | ||
| // supports HTTPS as well. | ||
| HttpURLConnection conn = (HttpURLConnection) url.openConnection(); | ||
| HttpURLConnection conn = (HttpURLConnection) uri.toURL().openConnection(); |
There was a problem hiding this comment.
note: Now if nextAsUri is provided with invalid scheme for URL we'll get MalformedURLException here instead. It's handled by catch but just wanted to make sure that's intended.
There was a problem hiding this comment.
Thanks for pointing this out, i will address it in next PRs
Looks to me like you have more experience in this than I do. I don't remember what are the finer differences between URL and URI, nor did I understand what is the motivation of this change (though I can guess it has something to do with encoding the parameters, I didn't understand why URL isn't good enough for that). @Bouncheck I see you left some review comments, please leave an approval when you are ok with the change and I can merge it. |
URL is not good enough, because we want to encode rack and datacenter parameters properly, and only URI does it. |
|
@Bouncheck, could you please take a look again. |
|
I don't see the new changes pushed |
dkropachev
force-pushed
the
dk/java-switch-url-to-uri
branch
from
f2b87eb to
a3661de
Compare
done |
dkropachev
force-pushed
the
dk/java-switch-url-to-uri
branch
2 times, most recently
from
6f4d1a3 to
05a4676
Compare
| return new URL(alternatorScheme, nextNode(), alternatorPort, file); | ||
| } catch (MalformedURLException e) { | ||
| return new URI(alternatorScheme, "", nextNode(), alternatorPort, file, "", ""); | ||
| } catch (URISyntaxException e) { | ||
| // Can only happen if alternatorScheme is an unknown one. | ||
| logger.log(Level.WARNING, "nextAsURL", e); |
There was a problem hiding this comment.
nit: nextAsURL inside logger.log( should change to nextAsURI
dkropachev
force-pushed
the
dk/java-switch-url-to-uri
branch
from
05a4676 to
6d5593d
Compare
thanks, fixed. |
We will need it later for #40