[
https://issues.apache.org/jira/browse/HDFS-5312?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13840647#comment-13840647
]
Jing Zhao commented on HDFS-5312:
---------------------------------
The patch looks good overall. Some comments:
# In DFSUtil#getInfoServer, we may want to throw an
IOException/RuntimeException instead of
IllegalArgumentException in the following code (or using URI.create() there):
{code}
+ } catch (URISyntaxException e) {
+ throw new IllegalArgumentException(e);
}
{code}
# Since you touched GetImageServlet.java, you can also clean the following code,
where we do not need to call getServletContext() again.
{code}
final Configuration conf =
(Configuration)getServletContext().getAttribute(JspHelper.CURRENT_CONF);
{code}
# In SecondaryNameNode#getInfoServer, the following change will change the
original behavior:
{code}
- private String getInfoServer() throws IOException {
+ private URL getInfoServer() throws IOException {
URI fsName = FileSystem.getDefaultUri(conf);
if (!HdfsConstants.HDFS_URI_SCHEME.equalsIgnoreCase(fsName.getScheme())) {
throw new IOException("This is not a DFS");
}
+ InetSocketAddress nnAddr = new InetSocketAddress(fsName.getHost(),
+ fsName.getPort());
- String configuredAddress = DFSUtil.getInfoServer(null, conf, false);
- String address = DFSUtil.substituteForWildcardAddress(configuredAddress,
- fsName.getHost());
- LOG.debug("Will connect to NameNode at HTTP address: " + address);
+ URL address = DFSUtil.getInfoServer(nnAddr, conf,
+ DFSUtil.getHttpClientScheme(conf)).toURL();
{code}
The original code will read http/https address from the configuration first,
and use filesystem default URI as a fallback in case of wildcard address. With
the change the filesystem default URI will become the first choice.
# Similarly we may want to use the original logic for
BootstrapStandby#parseConfAndFindOtherNN, and
StandbyCheckpointer#getHttpAddress(Configuration).
# Please update the jira title since the current patch not just refactors the
code.
> Refactor DFSUtil#getInfoServer to return an URI
> -----------------------------------------------
>
> Key: HDFS-5312
> URL: https://issues.apache.org/jira/browse/HDFS-5312
> Project: Hadoop HDFS
> Issue Type: Sub-task
> Reporter: Haohui Mai
> Assignee: Haohui Mai
> Attachments: HDFS-5312.000.patch, HDFS-5312.001.patch,
> HDFS-5312.002.patch, HDFS-5312.003.patch, HDFS-5312.004.patch,
> HDFS-5312.005.patch
>
>
> DFSUtil#getInfoServer() returns only the authority (i.e., host+port) when
> searching for the http / https server. This is insufficient because HDFS-5536
> and related jiras allows NN / DN / JN to open HTTPS only using the HTTPS_ONLY
> policy.
> This JIRA addresses two issues. First, DFSUtil#getInfoServer() should return
> an URI instead of a string, so that the scheme is an inherent parts of the
> return value, which eliminates the task of figuring out the scheme by design.
> Second, it introduces a new function to choose whether http or https should
> be used to connect to the remote server based of dfs.http.policy.
--
This message was sent by Atlassian JIRA
(v6.1#6144)
