accumulo-notifications mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From GitBox <...@apache.org>
Subject [GitHub] [accumulo] keith-turner commented on a change in pull request #1199: Fix #1122 Added concurrent structures to ThriftTransportPool
Date Wed, 12 Jun 2019 22:19:52 GMT
keith-turner commented on a change in pull request #1199: Fix #1122 Added concurrent structures
to ThriftTransportPool
URL: https://github.com/apache/accumulo/pull/1199#discussion_r293134436
 
 

 ##########
 File path: core/src/main/java/org/apache/accumulo/core/clientImpl/ThriftTransportPool.java
 ##########
 @@ -531,58 +533,57 @@ public void returnTransport(TTransport tsc) {
 
     ArrayList<CachedConnection> closeList = new ArrayList<>();
 
-    synchronized (this) {
-      CachedConnections cachedConns = getCache().get(ctsc.getCacheKey());
-      if (cachedConns != null) {
-        CachedConnection cachedConnection = cachedConns.reserved.remove(ctsc);
-        if (cachedConnection != null) {
-          if (ctsc.sawError) {
-            closeList.add(cachedConnection);
-
-            log.trace("Returned connection had error {}", ctsc.getCacheKey());
-
-            Long ecount = errorCount.get(ctsc.getCacheKey());
-            if (ecount == null)
-              ecount = 0L;
-            ecount++;
-            errorCount.put(ctsc.getCacheKey(), ecount);
-
-            Long etime = errorTime.get(ctsc.getCacheKey());
-            if (etime == null) {
-              errorTime.put(ctsc.getCacheKey(), System.currentTimeMillis());
-            }
+    // NOT NEEDED? synchronized (this) {
+    CachedConnections cachedConns = getCache().get(ctsc.getCacheKey());
+    if (cachedConns != null) {
+      CachedConnection cachedConnection = cachedConns.reserved.remove(ctsc);
+      if (cachedConnection != null) {
+        if (ctsc.sawError) {
+          closeList.add(cachedConnection);
 
-            if (ecount >= ERROR_THRESHOLD && !serversWarnedAbout.contains(ctsc.getCacheKey()))
{
-              log.warn(
-                  "Server {} had {} failures in a short time period, will not complain anymore",
-                  ctsc.getCacheKey(), ecount);
-              serversWarnedAbout.add(ctsc.getCacheKey());
-            }
+          log.trace("Returned connection had error {}", ctsc.getCacheKey());
+
+          Long ecount = errorCount.get(ctsc.getCacheKey());
 
 Review comment:
   The get and put on this map is susceptible to race conditions now that there is no synch.
 The merge function on a concurrent map may do this in a thread safe way, need to verify this.
 So may be able to replace it with following.
   
   ```java
   Long ecount = errorCount.merge(ctsc.getCacheKey(), 1L, Long::sum);
   ```
   
   Not completely sure the above code is correct, but  I think using merge is the way to go.

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


With regards,
Apache Git Services

Mime
View raw message