hbase-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From la...@apache.org
Subject svn commit: r1228759 - in /hbase/branches/0.90: CHANGES.txt src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java src/main/java/org/apache/hadoop/hbase/util/SoftValueSortedMap.java
Date Sun, 08 Jan 2012 01:14:06 GMT
Author: larsh
Date: Sun Jan  8 01:14:06 2012
New Revision: 1228759

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

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

Modified: hbase/branches/0.90/CHANGES.txt
URL: http://svn.apache.org/viewvc/hbase/branches/0.90/CHANGES.txt?rev=1228759&r1=1228758&r2=1228759&view=diff
==============================================================================
--- hbase/branches/0.90/CHANGES.txt (original)
+++ hbase/branches/0.90/CHANGES.txt Sun Jan  8 01:14:06 2012
@@ -12,6 +12,7 @@ Release 0.90.6 - Unreleased
                (Ramkrishna)
    HBASE-5009  Failure of creating split dir if it already exists prevents splits from happening
further
    HBASE-5041  Major compaction on non existing table does not throw error (Shrijeet)
+   HBASE-5088  A concurrency issue on SoftValueSortedMap (Jieshan Bean and Lars H)
 
   IMPROVEMENT
    HBASE-5102  Change the default value of the property "hbase.connection.per.config" to
false in

Modified: hbase/branches/0.90/src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java
URL: http://svn.apache.org/viewvc/hbase/branches/0.90/src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java?rev=1228759&r1=1228758&r2=1228759&view=diff
==============================================================================
--- hbase/branches/0.90/src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java
(original)
+++ hbase/branches/0.90/src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java
Sun Jan  8 01:14:06 2012
@@ -32,6 +32,7 @@ import java.util.Map;
 import java.util.NoSuchElementException;
 import java.util.Map.Entry;
 import java.util.Set;
+import java.util.SortedMap;
 import java.util.TreeSet;
 import java.util.concurrent.Callable;
 import java.util.concurrent.ConcurrentHashMap;
@@ -464,9 +465,9 @@ public class HConnectionManager {
      * Map of table to table {@link HRegionLocation}s.  The table key is made
      * by doing a {@link Bytes#mapKey(byte[])} of the table's name.
      */
-    private final Map<Integer, SoftValueSortedMap<byte [], HRegionLocation>>
+    private final Map<Integer, SortedMap<byte [], HRegionLocation>>
       cachedRegionLocations =
-        new HashMap<Integer, SoftValueSortedMap<byte [], HRegionLocation>>();
+        new HashMap<Integer, SortedMap<byte [], HRegionLocation>>();
 
     // region cache prefetch is enabled by default. this set contains all
     // tables whose region cache prefetch are disabled.
@@ -1036,7 +1037,7 @@ public class HConnectionManager {
      */
     HRegionLocation getCachedLocation(final byte [] tableName,
         final byte [] row) {
-      SoftValueSortedMap<byte [], HRegionLocation> tableLocations =
+      SortedMap<byte [], HRegionLocation> tableLocations =
         getTableLocations(tableName);
 
       // start to examine the cache. we can only do cache actions
@@ -1052,7 +1053,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,
@@ -1095,7 +1096,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.
@@ -1118,11 +1119,11 @@ public class HConnectionManager {
      * @param tableName
      * @return Map of cached locations for passed <code>tableName</code>
      */
-    private SoftValueSortedMap<byte [], HRegionLocation> getTableLocations(
+    private SortedMap<byte [], HRegionLocation> getTableLocations(
         final byte [] tableName) {
       // find the map of cached locations for this table
       Integer key = Bytes.mapKey(tableName);
-      SoftValueSortedMap<byte [], HRegionLocation> result;
+      SortedMap<byte [], HRegionLocation> result;
       synchronized (this.cachedRegionLocations) {
         result = this.cachedRegionLocations.get(key);
         // if tableLocations for this table isn't built yet, make one
@@ -1155,7 +1156,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);
       if (tableLocations.put(startKey, location) == null) {
         LOG.debug("Cached location for " +
@@ -1483,7 +1484,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.90/src/main/java/org/apache/hadoop/hbase/util/SoftValueSortedMap.java
URL: http://svn.apache.org/viewvc/hbase/branches/0.90/src/main/java/org/apache/hadoop/hbase/util/SoftValueSortedMap.java?rev=1228759&r1=1228758&r2=1228759&view=diff
==============================================================================
--- hbase/branches/0.90/src/main/java/org/apache/hadoop/hbase/util/SoftValueSortedMap.java
(original)
+++ hbase/branches/0.90/src/main/java/org/apache/hadoop/hbase/util/SoftValueSortedMap.java
Sun Jan  8 01:14:06 2012
@@ -19,16 +19,17 @@
  */
 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.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
@@ -40,7 +41,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() {
@@ -55,11 +57,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;
   }
 
   /**
@@ -68,127 +80,155 @@ 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() {
+  public Set<Map.Entry<K,V>> entrySet() {
     throw new RuntimeException("Not implemented");
   }
 
-  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 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> {
     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