[ https://issues.apache.org/jira/browse/HDFS-11546?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15993970#comment-15993970 ]
Chris Douglas commented on HDFS-11546: -------------------------------------- Thanks for the iterations, I think this is close. bq. How do we link to HADOOP-12077 bq. What kind of documentation would we add for the TODOs I don't think it needs an explicit link, I was just trying to understand the semantics the router implements. Users of router-based HDFS federation (R-BF) will want to understand what guarantees it provides for these calls without reading the code. Somewhere in the branch there should be a .md documenting how to configure and use R-BF, how the config knobs interact, limitations (e.g., no snapshots), etc. Filing a documentation subtask on HDFS-10467 will provide a place to track its impl. Comments on 006: * The {{ConnectionPool#isConnectionUsable}} method doesn't distinguish between a reference to a busy, closed, or even null connection. The caller is expected to handle all these cases, or are closed/busy the same case? Should the caller retry on null? * It looks like the connection could be returned to multiple threads before it's marked busy, since that only happens at {{ConnectionContext#getClient}}. ** The caller could receive a "non-busy" connection handle, only to have it closed before it can use it. ** Since {{busy}} is a boolean, those multiple threads will mark the thread as non-busy at the first call that releases it ** It's plausible that this will grow a pool of connections roughly proportional to load, but its semantics are fuzzy. * The synchronization in {{ConnectionPool}} is hard to follow. For example, {{connections}} is protected by the instance ({{#close}}), by the read lock ({{#getConnection}}), by a lock on the atomic int ({{#addingConnections}}). Even if this improves concurrency, it seems difficult to maintain without introducing deadlocks (I couldn't find any cycles in the existing code). ** Would it make sense for the {{ConnectionManager}} to mange the size of its {{ConnectionPool}} instances, rather than having each dynamically size itself? Imposing a cap on the total number of concurrent connections, QoS/quotas, etc. at the router probably requires some central management, and it could make the synchronization of the pool itself trivial (read lock for grabbing a connection from the pool, a write lock to add a thread from the manager ** The pattern of creating a thread to create the connection out-of-band is kind of heavy. If this were moved to the {{ConnectionManager}}, perhaps the decision could be inline and- if it's expensive- the connection creation could be offloaded to a single thread (pool) in the manager The rest of the patch looks OK. I can't really go through {{ClientProtocol}} in detail, but this looks fine as a base. I looked at {{getListing}} and {{create}}. The semantics for the multiple possible create locations, as discussed in HADOOP-12077 and elsewhere, are hard to reason about. If a remote location is temporarily unavailable, when it comes back clients may have a very strange view of the system, particularly retrying through separate routers which may have a partitioned view. It's an interesting capability to include, and for read-only paths replicated at all targets it's probably fine. If users have docs laying out how to use it (safely and dangerously), it seems fine to include even if it won't work seamlessly. > 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, > HDFS-11546-HDFS-10467-005.patch, HDFS-11546-HDFS-10467-006.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