hadoop-hdfs-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Xiao Chen (JIRA)" <j...@apache.org>
Subject [jira] [Comment Edited] (HDFS-12910) Secure Datanode Starter should log the port when it
Date Wed, 13 Dec 2017 05:16:00 GMT

    [ 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-unsubscribe@hadoop.apache.org
For additional commands, e-mail: hdfs-issues-help@hadoop.apache.org


Mime
View raw message