[GitHub] [hadoop] fengnanli commented on a change in pull request #2651: HDFS-15757 RBF: Improving Router Connection Management

[GitBox <gi...@apache.org>]

fengnanli commented on a change in pull request #2651:
URL: https://github.com/apache/hadoop/pull/2651#discussion_r634928450



##########
File path: hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/router/ConnectionContext.java
##########
@@ -83,30 +98,38 @@ public synchronized boolean isUsable() {
    */
   public synchronized ProxyAndInfo<?> getClient() {
     this.numThreads++;
+    this.lastActiveTs = Time.monotonicNow();
     return this.client;
   }
 
   /**
-   * Release this connection. If the connection was closed, close the proxy.
-   * Otherwise, mark the connection as not used by us anymore.
+   * Release this connection.
    */
   public synchronized void release() {
-    if (--this.numThreads == 0 && this.closed) {
-      close();
+    if (this.numThreads > 0) {
+      this.numThreads--;
     }
   }
 
   /**
-   * We will not use this connection anymore. If it's not being used, we close
-   * it. Otherwise, we let release() do it once we are done with it.
+   * Close a connection. Only idle connections can be closed since
+   * the RPC proxy would be shut down immediately.
+   *
+   * @param force whether the connection should be closed anyway.
+   * @throws IllegalStateException when the connection is not idle
    */
-  public synchronized void close() {
-    this.closed = true;
-    if (this.numThreads == 0) {
-      Object proxy = this.client.getProxy();
-      // Nobody should be using this anymore so it should close right away
-      RPC.stopProxy(proxy);
+  public synchronized void close(boolean force) throws IllegalStateException {
+    if (!force && this.numThreads > 0) {
+      throw new IllegalStateException("Active connection cannot be closed");

Review comment:
       Good point @DazhuangSu I made the change according to replace the exception with an error log (since there is not much we can do even we catch the exception).
   @Hexiaoqiao @goiri  Can we consider merging this one since it is there for a long time and there is no issue from some users' experience (except for the this suggestion which will be fixed.)




-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org