[
https://issues.apache.org/jira/browse/HDFS-11546?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15965226#comment-15965226
]
Chris Douglas commented on HDFS-11546:
--------------------------------------
Looks good, overall. A few questions:
* Many fields e.g., {{ConnectionManager.Timer}},
{{RouterRpcClient.retryPolicy}}, {{RouterRpcClient.executorService}} can be
final
* {{RemoteLocationContext#equals}} relying on hashcode equality of Strings is
too weak
* In {{ConnectionPool#getNumActiveConnections}}, instead of catching
AIOOBException: use a lock, COWArrayList, or other threadsafe collection.
* The threading in {{ConnectionPool}} generally seems wonky. It avoids locks,
but catches a lot of exceptions. Instead of synchronizing on a special {{Object
lock}}, most of this could synchronize on the {{connections}} field or use a
threadsafe collections.
* {{ConnectionPool}} seems to distribute requests to connections RR, adding new
connections (up to some limit) if it wraps while a connection is still "busy".
Is that right?
* {{ConnectionPool#cleanup}} should remove from the end of an ArrayList to
avoid copies, not the front (and be correctly synchronized)
* If the client in the {{ConnectionPool}} is configured to retry, and the
proxied client retries, that may go to a different connection in the pool (or a
different router), right? Should the proxy never retry to avoid repeating past
operations, or does some other mechanism prevent this?
* {{ConnectionPool#close}} doesn't seem to do any cleanup work (interrupting
threads, etc.). This is a "soft" shutdown?
* {{ConnectionManager}} uses {{Timer/TimerTask}}, which are sort-of deprecated
in favor of {{ScheduledThreadPoolExecutor}} after 1.5
* The {{ConnectionManager}} creates keys for each RPC proxy by creating a
String of the UGI and hashcode for each token. The chance of collision seems
remote, but unnecessarily non-zero. Instead of a flat {{String ->
ConnectionPool}} map, could this maintain a key of user/NN? Is there a
particular reason to include tokens as part of the key?
* Does {{RouterRpcClient#invokeSequential}} and
{{RouterRpcClient#invokeConcurrent}} implement functionality similar to
HADOOP-12077? There should probably be a documentation JIRA to describe common
patterns, limitations, and deployment. In particular, the subset of
{{ClientProtocol}} implemented by {{RouterRpcServer}} should be documented.
* I didn't review the details of {{RouterRpcServer}} since most of it seems to
be wrapping the client proxy, but there are some TODO items there. Those don't
need to block commit to the branch, but they should probably be documented or
addressed before merge.
* The defaults in {{hdfs-site.xml}} look conservative; the description could
include some cursory guidance on setting them.
* Thanks for adding the integration tests
> Federation Router RPC server
> ----------------------------
>
> Key: HDFS-11546
> URL: https://issues.apache.org/jira/browse/HDFS-11546
> Project: Hadoop HDFS
> Issue Type: Sub-task
> Components: hdfs
> Affects Versions: HDFS-10467
> Reporter: Inigo Goiri
> Assignee: Inigo Goiri
> Attachments: HDFS-11546-HDFS-10467-000.patch,
> HDFS-11546-HDFS-10467-001.patch, HDFS-11546-HDFS-10467-002.patch,
> HDFS-11546-HDFS-10467-003.patch, HDFS-11546-HDFS-10467-004.patch
>
>
> RPC server side of the Federation Router implements ClientProtocol.
--
This message was sent by Atlassian JIRA
(v6.3.15#6346)
---------------------------------------------------------------------
To unsubscribe, e-mail: hdfs-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: hdfs-issues-h...@hadoop.apache.org
