[
https://issues.apache.org/jira/browse/HDFS-12910?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16288725#comment-16288725
]
Xiao Chen edited comment on HDFS-12910 at 12/13/17 5:15 AM:
------------------------------------------------------------
Thanks for moving this forward, Nanda and Stephen! Latest patch looks pretty
good to me. Also thanks Nanda for the understanding and Stephen for the new
revs.
- Checkstyle warnings seem relevant, please fix those. Findbugs and unit test
failures looks unrelated here.
- bq. factor this out into one reusable method
Let's please do that. For readability it maybe better to still do this on 2
lines, so whoever read the code here doesn't have to check the extracted method
to figure out it's just rethrowing a BindException (as opposed to, say throw a
IOE or RTE).
{code}
BindException newBe = appendAddressToBindException();
throw newBe;
{code}
- In the test, I think we should be good to just verify the port number is
contained in the message - in case some funny dns mapping happens to the
jenkins server running the tests.
- Also saw one other minor thing not brought in by this patch, but would be
great if we can fix that too.
{code}
if (localAddr.getPort() != infoSocAddr.getPort()) {
throw new RuntimeException("Unable to bind on specified info port in
secure " +
"context. Needed " + streamingAddr.getPort() + ", got " +
ss.getLocalPort());
}
{code}
The messages should say {{... Needed " + infoSocAddr.getPort() ...}}.
To follow up a bit on the discussion:
Technically we can use a local var to temporarily save the current address
being bond, and just log that in the finally to have just 1 finally block.
Don't think that's necessary here though. Looking at the {{getSecureResources}}
method, I think it really should have been broken down into 2 methods:
openRpcPort which returns the {{ServerSocket ss}}, and openWebServerPort which
returns the {{ServerSocketChannel httpChannel}}. Since it's already written
this way, patch 4 should be good as-is so we don't unnecessarily change too
many lines solely for a log message improvement. :)
was (Author: xiaochen):
Thanks for moving this forward, Nanda and Stephen! Latest patch looks pretty
good to me. Also thanks Nanda for the understanding and Stephen for the new
revs.
Checkstyle warnings seem relevant, please fix those. Findbugs and unit test
failures looks unrelated here.
bq. factor this out into one reusable method
Let's please do that. For readability it maybe better to still do this on 2
lines, so whoever read the code here doesn't have to check the extracted method
to figure out it's just rethrowing a BindException (as opposed to, say throw a
IOE or RTE).
{code}
BindException newBe = appendAddressToBindException();
throw newBe;
{code}
Also saw one other minor thing not brought in by this patch, but would be great
if we can fix that too.
{code}
if (localAddr.getPort() != infoSocAddr.getPort()) {
throw new RuntimeException("Unable to bind on specified info port in
secure " +
"context. Needed " + streamingAddr.getPort() + ", got " +
ss.getLocalPort());
}
{code}
The messages should say {{... Needed " + infoSocAddr.getPort() ...}}.
To follow up a bit on the discussion:
Technically we can use a local var to temporarily save the current address
being bond, and just log that in the finally to have just 1 finally block.
Don't think that's necessary here though. Looking at the {{getSecureResources}}
method, I think it really should have been broken down into 2 methods:
openRpcPort which returns the {{ServerSocket ss}}, and openWebServerPort which
returns the {{ServerSocketChannel httpChannel}}. Since it's already written
this way, patch 4 should be good as-is so we don't unnecessarily change too
many lines solely for a log message improvement. :)
> Secure Datanode Starter should log the port when it
> ----------------------------------------------------
>
> Key: HDFS-12910
> URL: https://issues.apache.org/jira/browse/HDFS-12910
> Project: Hadoop HDFS
> Issue Type: Improvement
> Components: datanode
> Affects Versions: 3.1.0
> Reporter: Stephen O'Donnell
> Assignee: Stephen O'Donnell
> Priority: Minor
> Attachments: HDFS-12910.001.patch, HDFS-12910.002.patch,
> HDFS-12910.003.patch, HDFS-12910.004.patch
>
>
> When running a secure data node, the default ports it uses are 1004 and 1006.
> Sometimes other OS services can start on these ports causing the DN to fail
> to start (eg the nfs service can use random ports under 1024).
> When this happens an error is logged by jsvc, but it is confusing as it does
> not tell you which port it is having issues binding to, for example, when
> port 1004 is used by another process:
> {code}
> Initializing secure datanode resources
> java.net.BindException: Address already in use
> at sun.nio.ch.Net.bind0(Native Method)
> at sun.nio.ch.Net.bind(Net.java:433)
> at sun.nio.ch.Net.bind(Net.java:425)
> at
> sun.nio.ch.ServerSocketChannelImpl.bind(ServerSocketChannelImpl.java:223)
> at sun.nio.ch.ServerSocketAdaptor.bind(ServerSocketAdaptor.java:74)
> at
> org.apache.hadoop.hdfs.server.datanode.SecureDataNodeStarter.getSecureResources(SecureDataNodeStarter.java:105)
> at
> org.apache.hadoop.hdfs.server.datanode.SecureDataNodeStarter.init(SecureDataNodeStarter.java:71)
> at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
> at
> sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
> at
> sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
> at java.lang.reflect.Method.invoke(Method.java:498)
> at
> org.apache.commons.daemon.support.DaemonLoader.load(DaemonLoader.java:207)
> Cannot load daemon
> Service exit with a return value of 3
> {code}
> And when port 1006 is used:
> {code}
> Opened streaming server at /0.0.0.0:1004
> java.net.BindException: Address already in use
> at sun.nio.ch.Net.bind0(Native Method)
> at sun.nio.ch.Net.bind(Net.java:433)
> at sun.nio.ch.Net.bind(Net.java:425)
> at
> sun.nio.ch.ServerSocketChannelImpl.bind(ServerSocketChannelImpl.java:223)
> at sun.nio.ch.ServerSocketAdaptor.bind(ServerSocketAdaptor.java:74)
> at sun.nio.ch.ServerSocketAdaptor.bind(ServerSocketAdaptor.java:67)
> at
> org.apache.hadoop.hdfs.server.datanode.SecureDataNodeStarter.getSecureResources(SecureDataNodeStarter.java:129)
> at
> org.apache.hadoop.hdfs.server.datanode.SecureDataNodeStarter.init(SecureDataNodeStarter.java:71)
> at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
> at
> sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
> at
> sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
> at java.lang.reflect.Method.invoke(Method.java:498)
> at
> org.apache.commons.daemon.support.DaemonLoader.load(DaemonLoader.java:207)
> Cannot load daemon
> Service exit with a return value of 3
> {code}
> We should catch the BindException exception and log out the problem
> address:port and then re-throw the exception to make the problem more clear.
> I will upload a patch for this.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
---------------------------------------------------------------------
To unsubscribe, e-mail: hdfs-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: hdfs-issues-h...@hadoop.apache.org
