[ https://issues.apache.org/jira/browse/HDFS-13566?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16504987#comment-16504987 ]
Erik Krogen commented on HDFS-13566: ------------------------------------ Hey [~vagarychen], looks like great work. I have quite a few comments but they are all minor: # On Server L380, is there better way to check if the protocol is RPC? Should we just check if {{call instanceof RpcCall}}? Or at least, can we use a constant instead of "rpc"? That name must be defined somewhere. # Why the unnecessary cast to String on Server L1748? # {{additionalListener}} field is a map holding potentially multiple listeners, it should be renamed something like {{additionalListeners}} or {{additionalListenerMap}}. # {{additionalListener}} is only set once, in the constructor. Let's make it {{final}} and remove all of the null checks. Then all of the times you have a for-loop nested inside of an if can just become a for-loop (which will handle an empty map gracefully). # If {{addAdditionalListener}} is only visible for testing, why is it public rather than package-private? # Minor nit, the exception thrown within {{addAdditionalListener}} is currently {{"There is already listener binding to:" + port}}, it should be more like {{"There is already a listener bound to: " + port}} (grammar + spacing fix) # Should {{addAdditionalListener}} be {{synchronized}} ? # {{getAdditionalListenerAddress}} should be named {{getAdditionalListenerAddresses}} (same for the corresponding new methods in {{NameNode}} and {{NameNodeRpcServer}}). Also the Javadoc for it has some grammar mistakes, should be: {code:java} * Return the set of all the configured additional socket addresses NameNode * RPC is listening on. If there are none, or it is not configured at all, an empty * set is returned. * @return the set of all the additional addresses on which the * RPC server is listening. {code} # {{TestIPC}} L372, missing a space between for and opening parenthesis # Why is the default additional port 9001? The default port is 8020. Maybe the default additional should be 8021. # {{NameNode#getNameNodeAddressHostAdditionalPortString}} should have a Javadoc, particularly to describe its selection logic. Also, the comment is not quite right... I think it should say "arbitrary" rather than "random". There is not actually any randomness involved here (same for MiniDFSCluster L1980/1997). # Why the unnecessary cast to {{InetSocketAddress}} at NameNode L1070? You should either specify the correct array type within {{toArray}}, or use {{.iterator().next()}}. Also maybe use {{isEmpty()}} instead of {{size() == 0}} ? # "dfs.additional.rpc.server.enabled" doesn't seem like the right name for this key. No additional RPC server is started. It should just be about additional ports. # Why don't we support multiple configured additional listeners in the constructor of {{NameNodeRpcServer}} ? Everywhere else in the code supports them, and it would be trivial to do so here. # Bad import on {{MiniDFSCluster}} L51 # {{MiniDFSCluster}} L1997: "NameNode has multiple additional port are configured" should be "NameNode has multiple additional ports configured" # {{TestDistributedFileSystem}} L430, why is it necessary to set "fs.defaultFS" ? > Add configurable additional RPC listener to NameNode > ---------------------------------------------------- > > Key: HDFS-13566 > URL: https://issues.apache.org/jira/browse/HDFS-13566 > Project: Hadoop HDFS > Issue Type: Sub-task > Components: ipc > Reporter: Chen Liang > Assignee: Chen Liang > Priority: Major > Attachments: HDFS-13566.001.patch > > > This Jira aims to add the capability to NameNode to run additional > listener(s). Such that NameNode can be accessed from multiple ports. > Fundamentally, this Jira tries to extend ipc.Server to allow configured with > more listeners, binding to different ports, but sharing the same call queue > and the handlers. Useful when different clients are only allowed to access > certain different ports. Combined with HDFS-13547, this also allows different > ports to have different SASL security levels. -- This message was sent by Atlassian JIRA (v7.6.3#76005) --------------------------------------------------------------------- To unsubscribe, e-mail: hdfs-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: hdfs-issues-h...@hadoop.apache.org