[
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: [email protected]
For additional commands, e-mail: [email protected]
