hbase-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From la...@apache.org
Subject svn commit: r1228536 - in /hbase/branches/0.92: CHANGES.txt src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java src/main/java/org/apache/hadoop/hbase/util/SoftValueSortedMap.java
Date Sat, 07 Jan 2012 01:39:06 GMT
Author: larsh
Date: Sat Jan  7 01:39:06 2012
New Revision: 1228536

URL: http://svn.apache.org/viewvc?rev=1228536&view=rev
Log:
HBASE-5088 A concurrency issue on SoftValueSortedMap (Jieshan Bean and Lars H)

Modified:
    hbase/branches/0.92/CHANGES.txt
    hbase/branches/0.92/src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java
    hbase/branches/0.92/src/main/java/org/apache/hadoop/hbase/util/SoftValueSortedMap.java

Modified: hbase/branches/0.92/CHANGES.txt
URL: http://svn.apache.org/viewvc/hbase/branches/0.92/CHANGES.txt?rev=1228536&r1=1228535&r2=1228536&view=diff
==============================================================================
--- hbase/branches/0.92/CHANGES.txt (original)
+++ hbase/branches/0.92/CHANGES.txt Sat Jan  7 01:39:06 2012
@@ -508,6 +508,7 @@ Release 0.92.0 - Unreleased
                the one actually in the AssignmentManager thus making the region inaccessible.(Ram)
    HBASE-5081  Distributed log splitting deleteNode races against splitLog retry (Prakash)
    HBASE-4357  Region stayed in transition - in closing state (Ming Ma)
+   HBASE-5088  A concurrency issue on SoftValueSortedMap (Jieshan Bean and Lars H)
 
   TESTS
    HBASE-4492  TestRollingRestart fails intermittently

Modified: hbase/branches/0.92/src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java
URL: http://svn.apache.org/viewvc/hbase/branches/0.92/src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java?rev=1228536&r1=1228535&r2=1228536&view=diff
==============================================================================
--- hbase/branches/0.92/src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java
(original)
+++ hbase/branches/0.92/src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java
Sat Jan  7 01:39:06 2012
@@ -36,6 +36,7 @@ import java.util.Map;
 import java.util.Map.Entry;
 import java.util.NoSuchElementException;
 import java.util.Set;
+import java.util.SortedMap;
 import java.util.TreeMap;
 import java.util.concurrent.Callable;
 import java.util.concurrent.ConcurrentHashMap;
@@ -1062,7 +1063,7 @@ public class HConnectionManager {
 
       // Cut the cache so that we only get the part that could contain
       // regions that match our key
-      SoftValueSortedMap<byte[], HRegionLocation> matchingRegions =
+      SortedMap<byte[], HRegionLocation> matchingRegions =
         tableLocations.headMap(row);
 
       // if that portion of the map is empty, then we're done. otherwise,
@@ -1105,7 +1106,7 @@ public class HConnectionManager {
      */
     void deleteCachedLocation(final byte [] tableName, final byte [] row) {
       synchronized (this.cachedRegionLocations) {
-        SoftValueSortedMap<byte [], HRegionLocation> tableLocations =
+        Map<byte[], HRegionLocation> tableLocations =
             getTableLocations(tableName);
         // start to examine the cache. we can only do cache actions
         // if there's something in the cache for this table.
@@ -1137,8 +1138,8 @@ public class HConnectionManager {
         if (!cachedServers.contains(server)) {
           return;
         }
-        for (SoftValueSortedMap<byte[], HRegionLocation> tableLocations :
-            cachedRegionLocations.values()) {
+        for (Map<byte[], HRegionLocation> tableLocations :
+          cachedRegionLocations.values()) {
           for (Entry<byte[], HRegionLocation> e : tableLocations.entrySet()) {
             if (e.getValue().getServerAddress().toString().equals(server)) {
               tableLocations.remove(e.getKey());
@@ -1195,7 +1196,7 @@ public class HConnectionManager {
     private void cacheLocation(final byte [] tableName,
         final HRegionLocation location) {
       byte [] startKey = location.getRegionInfo().getStartKey();
-      SoftValueSortedMap<byte [], HRegionLocation> tableLocations =
+      Map<byte [], HRegionLocation> tableLocations =
         getTableLocations(tableName);
       boolean hasNewCache = false;
       synchronized (this.cachedRegionLocations) {
@@ -1663,7 +1664,7 @@ public class HConnectionManager {
     int getNumberOfCachedRegionLocations(final byte[] tableName) {
       Integer key = Bytes.mapKey(tableName);
       synchronized (this.cachedRegionLocations) {
-        SoftValueSortedMap<byte[], HRegionLocation> tableLocs =
+        Map<byte[], HRegionLocation> tableLocs =
           this.cachedRegionLocations.get(key);
 
         if (tableLocs == null) {

Modified: hbase/branches/0.92/src/main/java/org/apache/hadoop/hbase/util/SoftValueSortedMap.java
URL: http://svn.apache.org/viewvc/hbase/branches/0.92/src/main/java/org/apache/hadoop/hbase/util/SoftValueSortedMap.java?rev=1228536&r1=1228535&r2=1228536&view=diff
==============================================================================
--- hbase/branches/0.92/src/main/java/org/apache/hadoop/hbase/util/SoftValueSortedMap.java
(original)
+++ hbase/branches/0.92/src/main/java/org/apache/hadoop/hbase/util/SoftValueSortedMap.java
Sat Jan  7 01:39:06 2012
@@ -19,17 +19,18 @@
  */
 package org.apache.hadoop.hbase.util;
 
+import java.lang.ref.Reference;
 import java.lang.ref.ReferenceQueue;
 import java.lang.ref.SoftReference;
 import java.util.ArrayList;
 import java.util.Collection;
+import java.util.Collections;
 import java.util.Comparator;
 import java.util.LinkedHashSet;
 import java.util.Map;
 import java.util.Set;
 import java.util.SortedMap;
 import java.util.TreeMap;
-import java.util.TreeSet;
 
 /**
  * A SortedMap implementation that uses Soft Reference values
@@ -41,7 +42,8 @@ import java.util.TreeSet;
  */
 public class SoftValueSortedMap<K,V> implements SortedMap<K,V> {
   private final SortedMap<K, SoftValue<K,V>> internalMap;
-  private final ReferenceQueue rq = new ReferenceQueue();
+  private final ReferenceQueue<V> rq = new ReferenceQueue<V>();
+  private Object sync;
 
   /** Constructor */
   public SoftValueSortedMap() {
@@ -56,11 +58,21 @@ public class SoftValueSortedMap<K,V> imp
     this(new TreeMap<K, SoftValue<K,V>>(c));
   }
 
-  /** For headMap and tailMap support
-   * @param original object to wrap
+  /** Internal constructor
+   * @param original object to wrap and synchronize on
    */
   private SoftValueSortedMap(SortedMap<K,SoftValue<K,V>> original) {
+    this(original, original);
+  }
+
+  /** Internal constructor
+   * For headMap, tailMap, and subMap support
+   * @param original object to wrap
+   * @param sync object to synchronize on
+   */
+  private SoftValueSortedMap(SortedMap<K,SoftValue<K,V>> original, Object sync)
{
     this.internalMap = original;
+    this.sync = sync;
   }
 
   /**
@@ -69,133 +81,164 @@ public class SoftValueSortedMap<K,V> imp
    * Internally these call checkReferences on each access.
    * @return How many references cleared.
    */
+  @SuppressWarnings("unchecked")
   private int checkReferences() {
     int i = 0;
-    for (Object obj; (obj = this.rq.poll()) != null;) {
+    for (Reference<? extends V> ref; (ref = this.rq.poll()) != null;) {
       i++;
-      //noinspection unchecked
-      this.internalMap.remove(((SoftValue<K,V>)obj).key);
+      this.internalMap.remove(((SoftValue<K,V>)ref).key);
     }
     return i;
   }
 
-  public synchronized V put(K key, V value) {
-    checkReferences();
-    SoftValue<K,V> oldValue = this.internalMap.put(key,
-      new SoftValue<K,V>(key, value, this.rq));
-    return oldValue == null ? null : oldValue.get();
+  public V put(K key, V value) {
+    synchronized(sync) {
+      checkReferences();
+      SoftValue<K,V> oldValue = this.internalMap.put(key,
+        new SoftValue<K,V>(key, value, this.rq));
+      return oldValue == null ? null : oldValue.get();
+    }
   }
 
-  @SuppressWarnings("unchecked")
-  public synchronized void putAll(Map map) {
+  @Override
+  public void putAll(Map<? extends K, ? extends V> m) {
     throw new RuntimeException("Not implemented");
   }
 
-  @SuppressWarnings({"SuspiciousMethodCalls"})
-  public synchronized V get(Object key) {
-    checkReferences();
-    SoftValue<K,V> value = this.internalMap.get(key);
-    if (value == null) {
-      return null;
-    }
-    if (value.get() == null) {
-      this.internalMap.remove(key);
-      return null;
+  public V get(Object key) {
+    synchronized (sync) {
+      checkReferences();
+      SoftValue<K, V> value = this.internalMap.get(key);
+      if (value == null) {
+        return null;
+      }
+      if (value.get() == null) {
+        this.internalMap.remove(key);
+        return null;
+      }
+      return value.get();
     }
-    return value.get();
   }
 
-  public synchronized V remove(Object key) {
-    checkReferences();
-    SoftValue<K,V> value = this.internalMap.remove(key);
-    return value == null ? null : value.get();
+  public V remove(Object key) {
+    synchronized (sync) {
+      checkReferences();
+      SoftValue<K, V> value = this.internalMap.remove(key);
+      return value == null ? null : value.get();
+    }
   }
 
-  public synchronized boolean containsKey(Object key) {
-    checkReferences();
-    return this.internalMap.containsKey(key);
+  public boolean containsKey(Object key) {
+    synchronized(sync) {
+      checkReferences();
+      return this.internalMap.containsKey(key);
+    }
   }
 
-  public synchronized boolean containsValue(Object value) {
-/*    checkReferences();
-    return internalMap.containsValue(value);*/
+  public boolean containsValue(Object value) {
     throw new UnsupportedOperationException("Don't support containsValue!");
   }
 
-  public synchronized K firstKey() {
-    checkReferences();
-    return internalMap.firstKey();
+  public K firstKey() {
+    synchronized(sync) {
+      checkReferences();
+      return internalMap.firstKey();
+    }
   }
 
-  public synchronized K lastKey() {
-    checkReferences();
-    return internalMap.lastKey();
+  public K lastKey() {
+    synchronized(sync) {
+      checkReferences();
+      return internalMap.lastKey();
+    }
   }
 
-  public synchronized SoftValueSortedMap<K,V> headMap(K key) {
-    checkReferences();
-    return new SoftValueSortedMap<K,V>(this.internalMap.headMap(key));
+  public SoftValueSortedMap<K,V> headMap(K key) {
+    synchronized(sync) {
+      checkReferences();
+      return new SoftValueSortedMap<K,V>(this.internalMap.headMap(key), sync);
+    }
   }
 
-  public synchronized SoftValueSortedMap<K,V> tailMap(K key) {
-    checkReferences();
-    return new SoftValueSortedMap<K,V>(this.internalMap.tailMap(key));
+  public SoftValueSortedMap<K,V> tailMap(K key) {
+    synchronized(sync) {
+      checkReferences();
+      return new SoftValueSortedMap<K,V>(this.internalMap.tailMap(key), sync);
+    }
   }
 
-  public synchronized SoftValueSortedMap<K,V> subMap(K fromKey, K toKey) {
-    checkReferences();
-    return new SoftValueSortedMap<K,V>(this.internalMap.subMap(fromKey, toKey));
+  public SoftValueSortedMap<K,V> subMap(K fromKey, K toKey) {
+    synchronized(sync) {
+      checkReferences();
+      return new SoftValueSortedMap<K,V>(this.internalMap.subMap(fromKey,
+          toKey), sync);
+    }
   }
 
-  public synchronized boolean isEmpty() {
-    checkReferences();
-    return this.internalMap.isEmpty();
+  public boolean isEmpty() {
+    synchronized(sync) {
+      checkReferences();
+      return this.internalMap.isEmpty();
+    }
   }
 
-  public synchronized int size() {
-    checkReferences();
-    return this.internalMap.size();
+  public int size() {
+    synchronized(sync) {
+      checkReferences();
+      return this.internalMap.size();
+    }
   }
 
-  public synchronized void clear() {
-    checkReferences();
-    this.internalMap.clear();
+  public void clear() {
+    synchronized(sync) {
+      checkReferences();
+      this.internalMap.clear();
+    }
   }
 
-  public synchronized Set<K> keySet() {
-    checkReferences();
-    return this.internalMap.keySet();
+  public Set<K> keySet() {
+    synchronized(sync) {
+      checkReferences();
+      // this is not correct as per SortedMap contract (keySet should be
+      // modifiable)
+      // needed here so that another thread cannot modify the keyset
+      // without locking
+      return Collections.unmodifiableSet(this.internalMap.keySet());
+    }
   }
 
-  @SuppressWarnings("unchecked")
-  public synchronized Comparator comparator() {
+  public Comparator<? super K> comparator() {
     return this.internalMap.comparator();
   }
 
-  public synchronized Set<Map.Entry<K,V>> entrySet() {
-    checkReferences();
-    Set<Map.Entry<K, SoftValue<K, V>>> entries = this.internalMap.entrySet();
-    Set<Map.Entry<K, V>> realEntries = new LinkedHashSet<Map.Entry<K, V>>();
-    for (Map.Entry<K, SoftValue<K, V>> entry : entries) {
-      realEntries.add(entry.getValue());
-    }
-    return realEntries;
-  }
-
-  public synchronized Collection<V> values() {
-    checkReferences();
-    Collection<SoftValue<K,V>> softValues = this.internalMap.values();
-    ArrayList<V> hardValues = new ArrayList<V>();
-    for(SoftValue<K,V> softValue : softValues) {
-      hardValues.add(softValue.get());
+  public Set<Map.Entry<K,V>> entrySet() {
+    synchronized(sync) {
+      checkReferences();
+      // this is not correct as per SortedMap contract (entrySet should be
+      // backed by map)
+      Set<Map.Entry<K, V>> realEntries = new LinkedHashSet<Map.Entry<K,
V>>();
+      for (Map.Entry<K, SoftValue<K, V>> entry : this.internalMap.entrySet())
{
+        realEntries.add(entry.getValue());
+      }
+      return realEntries;
+    }
+  }
+
+  public Collection<V> values() {
+    synchronized (sync) {
+      checkReferences();
+      ArrayList<V> hardValues = new ArrayList<V>();
+      for (SoftValue<K, V> softValue : this.internalMap.values()) {
+        hardValues.add(softValue.get());
+      }
+      return hardValues;
     }
-    return hardValues;
   }
 
   private static class SoftValue<K,V> extends SoftReference<V> implements Map.Entry<K,
V> {
     final K key;
 
-    SoftValue(K key, V value, ReferenceQueue q) {
+    SoftValue(K key, V value, ReferenceQueue<V> q) {
       super(value, q);
       this.key = key;
     }



Mime
View raw message