[GitHub] [pulsar] AlphaWang commented on a diff in pull request #16165: [improve][client] [PIP-165] Auto release client useless connections

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

AlphaWang commented on code in PR #16165:
URL: https://github.com/apache/pulsar/pull/16165#discussion_r931019688


##########
pulsar-client/src/main/java/org/apache/pulsar/client/impl/ClientCnx.java:
##########
@@ -1265,4 +1278,165 @@ private void checkRequestTimeout() {
     }
 
     private static final Logger log = LoggerFactory.getLogger(ClientCnx.class);
+
+    /**
+     * Check client connection is now free. This method may change the state to idle.
+     * This method will not change the state to idle.

Review Comment:
   Shall we state more clear on whether his method WILL or WILL NOT change the state to idle? 



##########
pulsar-client/src/main/java/org/apache/pulsar/client/impl/ClientCnx.java:
##########
@@ -1265,4 +1278,165 @@ private void checkRequestTimeout() {
     }
 
     private static final Logger log = LoggerFactory.getLogger(ClientCnx.class);
+
+    /**
+     * Check client connection is now free. This method may change the state to idle.
+     * This method will not change the state to idle.
+     * @return true if the connection is eligible.
+     */
+    public boolean idleCheck(){
+        if (pendingRequests != null && !pendingRequests.isEmpty()){

Review Comment:
   Suggest adding a whitespace before `{` to follow the code style guide.



##########
pulsar-client/src/main/java/org/apache/pulsar/client/impl/ClientCnx.java:
##########
@@ -1265,4 +1278,165 @@ private void checkRequestTimeout() {
     }
 
     private static final Logger log = LoggerFactory.getLogger(ClientCnx.class);
+
+    /**
+     * Check client connection is now free. This method may change the state to idle.
+     * This method will not change the state to idle.
+     * @return true if the connection is eligible.
+     */
+    public boolean idleCheck(){
+        if (pendingRequests != null && !pendingRequests.isEmpty()){
+            return false;
+        }
+        if (waitingLookupRequests != null  && !waitingLookupRequests.isEmpty()){
+            return false;
+        }
+        if (!consumers.isEmpty()){
+            return false;
+        }
+        if (!producers.isEmpty()){
+            return false;
+        }
+        if (!transactionMetaStoreHandlers.isEmpty()){
+            return false;
+        }
+        return true;
+    }
+    /**
+     * Get idle-stat.
+     * @return connection idle-stat
+     */
+    public IdleState getIdleStat(){
+        return STATE_UPDATER.get(this);
+    }
+    /**
+     * Compare and switch idle-stat.
+     * @return Whether the update is successful.Because there may be other threads competing, possible return false.
+     */
+    public boolean compareAndSetIdleStat(IdleState originalStat, IdleState newStat){
+        return STATE_UPDATER.compareAndSet(this, originalStat, newStat);
+    }
+
+    /**
+     * Indicates the usage status of the connection and whether it has been released.
+     */
+    public enum IdleState {
+        /** The connection is in use. **/
+        USING,
+        /** The connection is in idle. **/
+        IDLE_MARKED,
+        /** The connection is in idle and will be released soon. **/
+        BEFORE_RELEASE,
+        /** The connection has already been released. **/
+        RELEASED;
+    }
+
+    /**
+     * @return Whether this connection is in use.
+     */
+    public boolean isUsing(){
+        return getIdleStat() == IdleState.USING;
+    }
+
+    /**
+     * @return Whether this connection is in idle.
+     */
+    public boolean isIdle(){
+        return getIdleStat() == IdleState.IDLE_MARKED;
+    }
+
+    /**
+     * @return Whether this connection is in idle and will be released soon.
+     */
+    public boolean isWillBeRelease(){
+        return getIdleStat() == IdleState.BEFORE_RELEASE;
+    }
+
+    /**
+     * @return Whether this connection has already been released.
+     */
+    public boolean alreadyRelease(){

Review Comment:
   Shall we keep the name consistent with other siblings `isXxx`?



##########
pulsar-client/src/main/java/org/apache/pulsar/client/impl/ClientCnx.java:
##########
@@ -1265,4 +1278,165 @@ private void checkRequestTimeout() {
     }
 
     private static final Logger log = LoggerFactory.getLogger(ClientCnx.class);
+
+    /**
+     * Check client connection is now free. This method may change the state to idle.
+     * This method will not change the state to idle.
+     * @return true if the connection is eligible.
+     */
+    public boolean idleCheck(){
+        if (pendingRequests != null && !pendingRequests.isEmpty()){
+            return false;
+        }
+        if (waitingLookupRequests != null  && !waitingLookupRequests.isEmpty()){
+            return false;
+        }
+        if (!consumers.isEmpty()){
+            return false;
+        }
+        if (!producers.isEmpty()){
+            return false;
+        }
+        if (!transactionMetaStoreHandlers.isEmpty()){
+            return false;
+        }
+        return true;
+    }
+    /**

Review Comment:
   May better to add an empty line before each method, to follow the code style standard.



-- 
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: commits-unsubscribe@pulsar.apache.org

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