goiri commented on code in PR #4531:
URL: https://github.com/apache/hadoop/pull/4531#discussion_r924855528
##########
hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/router/ConnectionPool.java:
##########
@@ -210,24 +217,22 @@ public AtomicInteger getClientIndex() {
* @return Connection context.
*/
protected ConnectionContext getConnection() {
-
this.lastActiveTime = Time.now();
-
- // Get a connection from the pool following round-robin
- ConnectionContext conn = null;
List<ConnectionContext> tmpConnections = this.connections;
- int size = tmpConnections.size();
- // Inc and mask off sign bit, lookup index should be non-negative int
- int threadIndex = this.clientIndex.getAndIncrement() & 0x7FFFFFFF;
- for (int i=0; i<size; i++) {
- int index = (threadIndex + i) % size;
- conn = tmpConnections.get(index);
- if (conn != null && conn.isUsable()) {
- return conn;
+ for (ConnectionContext tmpConnection : tmpConnections) {
+ if (tmpConnection != null && tmpConnection.isUsable()) {
+ return tmpConnection;
}
}
- // We return a connection even if it's active
+ ConnectionContext conn = null;
+ // We return a connection even if it's busy
+ int size = tmpConnections.size();
+ if (size > 0) {
+ // Get a connection from the pool following round-robin
+ int threadIndex = this.clientIndex.getAndIncrement() & 0x7FFFFFFF;
Review Comment:
We should keep the old comment:
```
// Inc and mask off sign bit, lookup index should be non-negative int
```
##########
hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/router/ConnectionPool.java:
##########
@@ -210,24 +217,22 @@ public AtomicInteger getClientIndex() {
* @return Connection context.
*/
protected ConnectionContext getConnection() {
-
this.lastActiveTime = Time.now();
-
- // Get a connection from the pool following round-robin
- ConnectionContext conn = null;
List<ConnectionContext> tmpConnections = this.connections;
- int size = tmpConnections.size();
- // Inc and mask off sign bit, lookup index should be non-negative int
- int threadIndex = this.clientIndex.getAndIncrement() & 0x7FFFFFFF;
- for (int i=0; i<size; i++) {
- int index = (threadIndex + i) % size;
- conn = tmpConnections.get(index);
- if (conn != null && conn.isUsable()) {
- return conn;
+ for (ConnectionContext tmpConnection : tmpConnections) {
Review Comment:
Is there something more efficient we can do for this?
Like sorting the list when we make the connection usable?
It is not that expensive to do this but just in case.
##########
hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/resources/hdfs-rbf-default.xml:
##########
@@ -134,6 +134,22 @@
</description>
</property>
+ <property>
+ <name>dfs.federation.router.enable.multiple.socket</name>
+ <value>false</value>
+ <description>
+ If enable multiple downstream socket or not.
Review Comment:
https://github.com/apache/hadoop/blob/trunk/hadoop-hdfs-project/hadoop-hdfs-rbf/src/site/markdown/HDFSRouterFederation.md
##########
hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/resources/hdfs-rbf-default.xml:
##########
@@ -134,6 +134,22 @@
</description>
</property>
+ <property>
+ <name>dfs.federation.router.enable.multiple.socket</name>
+ <value>false</value>
+ <description>
+ If enable multiple downstream socket or not.
Review Comment:
We should explain the relation between
dfs.federation.router.enable.multiple.socket and
dfs.federation.router.max.concurrency.per.connection.
In general it would be good to have this in some of the RBF md files
explaining why doing this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
