HBase
  1. HBase
  2. HBASE-5388

Tune HConnectionManager#getCachedLocation method

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.90.5
    • Fix Version/s: 0.94.0
    • Component/s: None
    • Labels:
    • Hadoop Flags:
      Reviewed
    • Tags:
      tunning getCachedLocation method

      Description

      About 75% improvement in execution time.
      1. Add the following method in SoftValueSortedMap:

      public synchronized <K, V> Entry<K, V> lowerEntry(K key) {
        return ((TreeMap) this.internalMap).lowerEntry(key);
      }
      

      2. Modify getCachedLocation:

       
      Map.Entry<byte[], HRegionLocation> tEntry = tableLocations.lowerEntry(row);
        if (tEntry != null) {
        HRegionLocation possibleRegion = tEntry.getValue();
        //other code
      }
      
      1. HConnectionManager.java
        58 kB
        ronghai.ma
      2. SoftValueSortedMap.java
        6 kB
        ronghai.ma
      3. SoftValueSortedMap.java
        6 kB
        ronghai.ma
      4. 5388-v2.txt
        5 kB
        Ted Yu
      5. 5388-v3.txt
        4 kB
        Ted Yu
      6. 5388-v4.txt
        6 kB
        Lars Hofhansl
      7. 5388-v5.txt
        5 kB
        Ted Yu
      8. 5388-v6.txt
        6 kB
        Ted Yu

        Activity

        Hide
        Ted Yu added a comment -

        @Ronghai:
        Can you upload a patch ?
        The improvement is measured in execution time, right ?

        Show
        Ted Yu added a comment - @Ronghai: Can you upload a patch ? The improvement is measured in execution time, right ?
        Hide
        ronghai.ma added a comment -

        My pleasure, and two questions:
        1. the svn location
        2. my account

        Yes, measeured in excution time.

        Show
        ronghai.ma added a comment - My pleasure, and two questions: 1. the svn location 2. my account Yes, measeured in excution time.
        Hide
        ronghai.ma added a comment -

        About 75% improvement。

        Show
        ronghai.ma added a comment - About 75% improvement。
        Hide
        Ted Yu added a comment -

        Here you can find source location:
        http://hbase.apache.org/source-repository.html

        Show
        Ted Yu added a comment - Here you can find source location: http://hbase.apache.org/source-repository.html
        Hide
        Lars Hofhansl added a comment -

        Wow, nice find. Should consider back porting at least to 0.92

        Show
        Lars Hofhansl added a comment - Wow, nice find. Should consider back porting at least to 0.92
        Hide
        Ted Yu added a comment -

        @Ronghai:
        I haven't seen your patch yet.
        According to your description in the JIRA, I am guessing something similar to the following:

        @@ -1067,9 +1067,12 @@
                 return null;
               }
        
        -      HRegionLocation rl = tableLocations.get(row);
        -      if (rl != null) {
        -        return rl;
        +      Map.Entry<byte[], HRegionLocation> tEntry = tableLocations.lowerEntry(row);
        +      if (tEntry != null) {
        +        HRegionLocation rl = tEntry.getValue();
        +        if (rl != null) {
        +          return rl;
        +        }
               }
        

        There is more code in getCachedLocation() following the above lines, revolving around tableLocations.headMap(row)
        I wonder how you measured the performance boost - by instrumenting getCachedLocation() I assume ?

        Show
        Ted Yu added a comment - @Ronghai: I haven't seen your patch yet. According to your description in the JIRA, I am guessing something similar to the following: @@ -1067,9 +1067,12 @@ return null ; } - HRegionLocation rl = tableLocations.get(row); - if (rl != null ) { - return rl; + Map.Entry< byte [], HRegionLocation> tEntry = tableLocations.lowerEntry(row); + if (tEntry != null ) { + HRegionLocation rl = tEntry.getValue(); + if (rl != null ) { + return rl; + } } There is more code in getCachedLocation() following the above lines, revolving around tableLocations.headMap(row) I wonder how you measured the performance boost - by instrumenting getCachedLocation() I assume ?
        Hide
        ronghai.ma added a comment -

        I will upload the patch latter, many meetings,take a nap.
        I reviewed the code, and changed some code.

        //SoftValueSortedMap#lowerKey
        public synchronized K lowerKey(K key) {
          checkReferences();    
          return ((TreeMap<K, SoftValue<K,V>>)this.internalMap).lowerKey(key);
        }
        //SoftValueSortedMap#getLower
        public synchronized V getLower(K key) {
          checkReferences(); 
          Map.Entry<K,SoftValue<K,V>> entry = ((TreeMap<K, SoftValue<K,V>>)this.internalMap).lowerEntry(key);
          if(entry==null){
            return null;
          }
          SoftValue<K,V> value=entry.getValue();
          if(value==null){
            return null;
          } 
          if (value.get() == null) {
            this.internalMap.remove(key);
            return null;
          }
          return value.get();
        }
        
        //HConnectionManager#HConnectionImplementation#getCachedLocation
        HRegionLocation rl = tableLocations.get(row);
        if (rl != null) {
          return rl;
        }
        rl = tableLocations.getLower(row);
        if(r1==null){
          return null;
        }
        byte[] endKey = rl.getRegionInfo().getEndKey();
        if(Bytes.equals(endKey,HConstants.EMPTY_END_ROW)||
          KeyValue.getRowComparator(tableName).compareRows(endKey, 0, endKey.length, row, 0, row.length) > 0){
          return rl;
        }
        
        Show
        ronghai.ma added a comment - I will upload the patch latter, many meetings,take a nap. I reviewed the code, and changed some code. //SoftValueSortedMap#lowerKey public synchronized K lowerKey(K key) { checkReferences(); return ((TreeMap<K, SoftValue<K,V>>) this .internalMap).lowerKey(key); } //SoftValueSortedMap#getLower public synchronized V getLower(K key) { checkReferences(); Map.Entry<K,SoftValue<K,V>> entry = ((TreeMap<K, SoftValue<K,V>>) this .internalMap).lowerEntry(key); if (entry== null ){ return null ; } SoftValue<K,V> value=entry.getValue(); if (value== null ){ return null ; } if (value.get() == null ) { this .internalMap.remove(key); return null ; } return value.get(); } //HConnectionManager#HConnectionImplementation#getCachedLocation HRegionLocation rl = tableLocations.get(row); if (rl != null ) { return rl; } rl = tableLocations.getLower(row); if (r1== null ){ return null ; } byte [] endKey = rl.getRegionInfo().getEndKey(); if (Bytes.equals(endKey,HConstants.EMPTY_END_ROW)|| KeyValue.getRowComparator(tableName).compareRows(endKey, 0, endKey.length, row, 0, row.length) > 0){ return rl; }
        Hide
        Ted Yu added a comment -

        I see.

        Should this new method, getLower(), be called getLowerValue() ? It returns the value portion of the entry.

        Show
        Ted Yu added a comment - I see. Should this new method, getLower(), be called getLowerValue() ? It returns the value portion of the entry.
        Hide
        Ted Yu added a comment -

        Please also publish performance numbers before and after the optimization.

        Thanks

        Show
        Ted Yu added a comment - Please also publish performance numbers before and after the optimization. Thanks
        Hide
        Lars Hofhansl added a comment -

        NavigableMap has lowerKey and lowerEntry, we should use the same naming scheme.
        (Ideally SoftValueSortedMap should implement NavigableMap. But that is a different story.)

        This whole block:

              SortedMap<byte[], HRegionLocation> matchingRegions =
                tableLocations.headMap(row);
        
              // if that portion of the map is empty, then we're done. otherwise,
              // we need to examine the cached location to verify that it is
              // a match by end key as well.
              if (!matchingRegions.isEmpty()) {
                HRegionLocation possibleRegion = null;
                try {
                  possibleRegion = matchingRegions.get(matchingRegions.lastKey());
        

        is essentially equivalent to

        possibleRegion = tableLocations.lowerEntry(row).getValue()
        

        @ronghai: I'm happy to make a patch.

        Show
        Lars Hofhansl added a comment - NavigableMap has lowerKey and lowerEntry, we should use the same naming scheme. (Ideally SoftValueSortedMap should implement NavigableMap. But that is a different story.) This whole block: SortedMap< byte [], HRegionLocation> matchingRegions = tableLocations.headMap(row); // if that portion of the map is empty, then we're done. otherwise, // we need to examine the cached location to verify that it is // a match by end key as well. if (!matchingRegions.isEmpty()) { HRegionLocation possibleRegion = null ; try { possibleRegion = matchingRegions.get(matchingRegions.lastKey()); is essentially equivalent to possibleRegion = tableLocations.lowerEntry(row).getValue() @ronghai: I'm happy to make a patch.
        Hide
        ronghai.ma added a comment -

        patch files

        Show
        ronghai.ma added a comment - patch files
        Hide
        ronghai.ma added a comment -

        rename the method SoftValueSortedMap#getLower to SoftValueSortedMap#getLowerValueByKey

        public synchronized V getLowerValueByKey(K key) {
        	checkReferences();
        	if(!(this.internalMap instanceof java.util.NavigableMap)){
        	  SortedMap<K,SoftValue<K,V>> sortedMap = this.internalMap.headMap(key);
        	  if(sortedMap==null){
        		return null;
        	  }
        	  SoftValue<K,V> value = sortedMap.get(sortedMap.lastKey());
        	  if(value==null){
        		return null;
        	  }
        	  if (value.get() == null) {
        	    this.internalMap.remove(key);
        	    return null;
        	  }
        	  return value.get();
        	}
        	
        	Map.Entry<K,SoftValue<K,V>> entry = ((TreeMap<K, SoftValue<K,V>>)this.internalMap).lowerEntry(key);
        	if(entry==null){
        		return null;
        	}
        	SoftValue<K,V> value=entry.getValue();
        	if(value==null){
        		return null;
        	} 
        	if (value.get() == null) {
              this.internalMap.remove(key);
              return null;
            }
        	return value.get();
          }
        
        Show
        ronghai.ma added a comment - rename the method SoftValueSortedMap#getLower to SoftValueSortedMap#getLowerValueByKey public synchronized V getLowerValueByKey(K key) { checkReferences(); if (!( this .internalMap instanceof java.util.NavigableMap)){ SortedMap<K,SoftValue<K,V>> sortedMap = this .internalMap.headMap(key); if (sortedMap== null ){ return null ; } SoftValue<K,V> value = sortedMap.get(sortedMap.lastKey()); if (value== null ){ return null ; } if (value.get() == null ) { this .internalMap.remove(key); return null ; } return value.get(); } Map.Entry<K,SoftValue<K,V>> entry = ((TreeMap<K, SoftValue<K,V>>) this .internalMap).lowerEntry(key); if (entry== null ){ return null ; } SoftValue<K,V> value=entry.getValue(); if (value== null ){ return null ; } if (value.get() == null ) { this .internalMap.remove(key); return null ; } return value.get(); }
        Hide
        Lars Hofhansl added a comment -

        Usually a diff is better: you can use "svn diff" to do that.
        Let's have a more standard lowerEntry rather then getLower.

        Show
        Lars Hofhansl added a comment - Usually a diff is better: you can use "svn diff" to do that. Let's have a more standard lowerEntry rather then getLower.
        Hide
        ronghai.ma added a comment -

        CPU:IA32
        JVM OPTIONS:-server
        cached regions:60000
        region row: 50000th region
        prewarm each method: 1,000,000 times
        run each method: 1,000,000 times
        Time Usage:
        By headMap(),lastKey(),get():1000ms
        By lowerEntry(K):600ms

        Show
        ronghai.ma added a comment - CPU:IA32 JVM OPTIONS:-server cached regions:60000 region row: 50000th region prewarm each method: 1,000,000 times run each method: 1,000,000 times Time Usage: By headMap(),lastKey(),get():1000ms By lowerEntry(K):600ms
        Hide
        ronghai.ma added a comment -

        @Lars hofhansl
        thank you.

        Show
        ronghai.ma added a comment - @Lars hofhansl thank you.
        Hide
        ronghai.ma added a comment -

        rename the method's name and change the TreeMap to NavigableMap of the if condition

        Show
        ronghai.ma added a comment - rename the method's name and change the TreeMap to NavigableMap of the if condition
        Hide
        Ted Yu added a comment -

        Patch v2 is adapted from the files Ronghai attached.
        I think the following check in SoftValueSortedMap.java isn't needed:

            if (!(this.internalMap instanceof NavigableMap)) {
        

        because the two SoftValueSortedMap ctor's which don't instantiate TreeMap are private.

        I included the if block for completeness.

        Show
        Ted Yu added a comment - Patch v2 is adapted from the files Ronghai attached. I think the following check in SoftValueSortedMap.java isn't needed: if (!( this .internalMap instanceof NavigableMap)) { because the two SoftValueSortedMap ctor's which don't instantiate TreeMap are private. I included the if block for completeness.
        Hide
        Ted Yu added a comment -

        @Ronghai:
        Since this is an improvement, we should start from HBase TRUNK first.

        Please generate patch from the root of your workspace by typing:

        svn diff
        

        Also, refer to HBASE-3678 for Eclipse formatter which takes care of formatting.

        Keep a space between if and left parenthesis, a space between right parenthesis and left curly brace.

        Show
        Ted Yu added a comment - @Ronghai: Since this is an improvement, we should start from HBase TRUNK first. Please generate patch from the root of your workspace by typing: svn diff Also, refer to HBASE-3678 for Eclipse formatter which takes care of formatting. Keep a space between if and left parenthesis, a space between right parenthesis and left curly brace.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12514315/5388-v2.txt
        against trunk revision .

        +1 @author. The patch does not contain any @author tags.

        -1 tests included. The patch doesn't appear to include any new or modified tests.
        Please justify why no new tests are needed for this patch.
        Also please list what manual steps were performed to verify this patch.

        -1 javadoc. The javadoc tool appears to have generated -136 warning messages.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        -1 findbugs. The patch appears to introduce 157 new Findbugs (version 1.3.9) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        -1 core tests. The patch failed these unit tests:
        org.apache.hadoop.hbase.mapreduce.TestHFileOutputFormat
        org.apache.hadoop.hbase.mapred.TestTableMapReduce
        org.apache.hadoop.hbase.io.hfile.TestForceCacheImportantBlocks
        org.apache.hadoop.hbase.mapreduce.TestImportTsv

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/948//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/948//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/948//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12514315/5388-v2.txt against trunk revision . +1 @author. The patch does not contain any @author tags. -1 tests included. The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. -1 javadoc. The javadoc tool appears to have generated -136 warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 157 new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed these unit tests: org.apache.hadoop.hbase.mapreduce.TestHFileOutputFormat org.apache.hadoop.hbase.mapred.TestTableMapReduce org.apache.hadoop.hbase.io.hfile.TestForceCacheImportantBlocks org.apache.hadoop.hbase.mapreduce.TestImportTsv Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/948//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/948//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/948//console This message is automatically generated.
        Hide
        Lars Hofhansl added a comment -

        Not too fond of this part:

        -      SortedMap<byte [], HRegionLocation> tableLocations =
        -        getTableLocations(tableName);
        +      SoftValueSortedMap<byte [], HRegionLocation> tableLocations =
        +        (SoftValueSortedMap) getTableLocations(tableName);
        

        I did a patch earlier that removed all concrete references to SoftValueSortedMap, except for creation... This would be a step back.
        Maybe SoftValueSortedMap should implement NavigableMap and just throw UnsupportedOperation on all other new methods, in that case SoftValueSortedMap could implement lowerEntry.

        ... Or maybe this would be overkill.

        Show
        Lars Hofhansl added a comment - Not too fond of this part: - SortedMap< byte [], HRegionLocation> tableLocations = - getTableLocations(tableName); + SoftValueSortedMap< byte [], HRegionLocation> tableLocations = + (SoftValueSortedMap) getTableLocations(tableName); I did a patch earlier that removed all concrete references to SoftValueSortedMap, except for creation... This would be a step back. Maybe SoftValueSortedMap should implement NavigableMap and just throw UnsupportedOperation on all other new methods, in that case SoftValueSortedMap could implement lowerEntry. ... Or maybe this would be overkill.
        Hide
        Ted Yu added a comment -

        It wouldn't be an overkill if you have strong belief.

        Show
        Ted Yu added a comment - It wouldn't be an overkill if you have strong belief.
        Hide
        ronghai.ma added a comment -

        @Lars Hofhansl
        I prefer to make code short, and let the instance thin.
        If SoftValueSortedMap implements NavigableMap, there will be more infomation in method_info and interfaces, and the interface method reference.

        @Zhihong Yu
        Thank you.
        Kill the code which is not usefull.

        Show
        ronghai.ma added a comment - @Lars Hofhansl I prefer to make code short, and let the instance thin. If SoftValueSortedMap implements NavigableMap, there will be more infomation in method_info and interfaces, and the interface method reference. @Zhihong Yu Thank you. Kill the code which is not usefull.
        Hide
        stack added a comment -

        I agree w/ Zhihong, that '+ if (!(this.internalMap instanceof NavigableMap)) {' block seems unnecessary.

        Is the 'greatest' in this right?

        +   * retrieves the value associated with the greatest key strictly less than
        +   *  the given key, or null if there is no such key
        

        Its unfortunate that SoftValueSortedMap sneaks back into HCM but I can live with it if no alternative (Lars' suggestion having SVSM implement NM didn't seem to bad but understand if it adds a bunch of boiler plate)

        Show
        stack added a comment - I agree w/ Zhihong, that '+ if (!(this.internalMap instanceof NavigableMap)) {' block seems unnecessary. Is the 'greatest' in this right? + * retrieves the value associated with the greatest key strictly less than + * the given key, or null if there is no such key Its unfortunate that SoftValueSortedMap sneaks back into HCM but I can live with it if no alternative (Lars' suggestion having SVSM implement NM didn't seem to bad but understand if it adds a bunch of boiler plate)
        Hide
        Ted Yu added a comment -

        The javadoc involving 'greatest' comes from javadoc for lowerEntry().

        I will attach a new patch without the instanceof block.

        Show
        Ted Yu added a comment - The javadoc involving 'greatest' comes from javadoc for lowerEntry(). I will attach a new patch without the instanceof block.
        Hide
        Ted Yu added a comment -

        Patch v3 aligns lowerValueByKey() with the rest of methods in SoftValueSortedMap.java

        Show
        Ted Yu added a comment - Patch v3 aligns lowerValueByKey() with the rest of methods in SoftValueSortedMap.java
        Hide
        stack added a comment -

        The javadoc involving 'greatest' comes from javadoc for lowerEntry().

        Ok

        v3 is good by me except for the HCM pollution w/ SoftValueSortedMap. I looked at making it implement NavigableMap and its 18 extra methods. They could all throw unsupported as per Lars. I'm fine w/ this patch though.

        Show
        stack added a comment - The javadoc involving 'greatest' comes from javadoc for lowerEntry(). Ok v3 is good by me except for the HCM pollution w/ SoftValueSortedMap. I looked at making it implement NavigableMap and its 18 extra methods. They could all throw unsupported as per Lars. I'm fine w/ this patch though.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12514383/5388-v3.txt
        against trunk revision .

        +1 @author. The patch does not contain any @author tags.

        -1 tests included. The patch doesn't appear to include any new or modified tests.
        Please justify why no new tests are needed for this patch.
        Also please list what manual steps were performed to verify this patch.

        -1 javadoc. The javadoc tool appears to have generated -136 warning messages.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        -1 findbugs. The patch appears to introduce 157 new Findbugs (version 1.3.9) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        -1 core tests. The patch failed these unit tests:
        org.apache.hadoop.hbase.io.hfile.TestHFileBlock
        org.apache.hadoop.hbase.mapreduce.TestImportTsv
        org.apache.hadoop.hbase.mapred.TestTableMapReduce
        org.apache.hadoop.hbase.mapreduce.TestHFileOutputFormat

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/952//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/952//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/952//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12514383/5388-v3.txt against trunk revision . +1 @author. The patch does not contain any @author tags. -1 tests included. The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. -1 javadoc. The javadoc tool appears to have generated -136 warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 157 new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed these unit tests: org.apache.hadoop.hbase.io.hfile.TestHFileBlock org.apache.hadoop.hbase.mapreduce.TestImportTsv org.apache.hadoop.hbase.mapred.TestTableMapReduce org.apache.hadoop.hbase.mapreduce.TestHFileOutputFormat Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/952//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/952//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/952//console This message is automatically generated.
        Hide
        Ted Yu added a comment -

        @Lars:
        What do you think ?

        Show
        Ted Yu added a comment - @Lars: What do you think ?
        Hide
        Ted Yu added a comment -

        The new method, lowerValueByKey(), isn't in NavigableMap.
        If we restrict SoftValueSortedMap to only implementing NavigableMap, where should we place lowerValueByKey() ?
        I assume we may find other place(s) in our code which can reuse lowerValueByKey().

        Show
        Ted Yu added a comment - The new method, lowerValueByKey(), isn't in NavigableMap. If we restrict SoftValueSortedMap to only implementing NavigableMap, where should we place lowerValueByKey() ? I assume we may find other place(s) in our code which can reuse lowerValueByKey().
        Hide
        ronghai.ma added a comment -

        rename it lower()

        Show
        ronghai.ma added a comment - rename it lower()
        Hide
        Ted Yu added a comment -

        lower() isn't in NavigableMap or TreeMap.
        And the name is not informative.

        Show
        Ted Yu added a comment - lower() isn't in NavigableMap or TreeMap. And the name is not informative.
        Hide
        ronghai.ma added a comment -

        As the class is called SoftValueSortedMap, how about SoftValueTreeMap or SoftValueNavigableMap

        Show
        ronghai.ma added a comment - As the class is called SoftValueSortedMap, how about SoftValueTreeMap or SoftValueNavigableMap
        Hide
        Lars Hofhansl added a comment -

        I had a look. Having SoftValueSortedMap implement NavigableMap just opens up a new can of worms.
        NavigableMap has lowerEntry, but what should that return? A SoftValue? That means the value of the returned entry can become null at any point.

        Let's go with the original patch and just add the reference to the concrete SoftValueSortedMap type.

        Show
        Lars Hofhansl added a comment - I had a look. Having SoftValueSortedMap implement NavigableMap just opens up a new can of worms. NavigableMap has lowerEntry, but what should that return? A SoftValue? That means the value of the returned entry can become null at any point. Let's go with the original patch and just add the reference to the concrete SoftValueSortedMap type.
        Hide
        Ted Yu added a comment -

        @Lars:
        Can you clarify which patch you favor: v2 or v3 ?

        Show
        Ted Yu added a comment - @Lars: Can you clarify which patch you favor: v2 or v3 ?
        Hide
        ronghai.ma added a comment -

        @Lars,Zhihong Yu
        What about a new Class for the cache, there are too many synchronized and checkReferences in SoftValueSortedMap.

        Show
        ronghai.ma added a comment - @Lars,Zhihong Yu What about a new Class for the cache, there are too many synchronized and checkReferences in SoftValueSortedMap.
        Hide
        Lars Hofhansl added a comment -

        @Ted: v3
        I'd probably keep the name of possibleRegion (rather than rl), also keep this comment

        -          // make sure that the end key is greater than the row we're looking
        -          // for, otherwise the row actually belongs in the next region, not
        -          // this one. the exception case is when the endkey is
        -          // HConstants.EMPTY_START_ROW, signifying that the region we're
        -          // checking is actually the last region in the table.
        

        which is still valid.
        Also is this needed:

        +      if (value.get() == null) {
        +        this.internalMap.remove(key);
        +        return null;
        +      }
        

        ? That is what checkReferences will do on the next access anyway, right?

        Show
        Lars Hofhansl added a comment - @Ted: v3 I'd probably keep the name of possibleRegion (rather than rl), also keep this comment - // make sure that the end key is greater than the row we're looking - // for , otherwise the row actually belongs in the next region, not - // this one. the exception case is when the endkey is - // HConstants.EMPTY_START_ROW, signifying that the region we're - // checking is actually the last region in the table. which is still valid. Also is this needed: + if (value.get() == null ) { + this .internalMap.remove(key); + return null ; + } ? That is what checkReferences will do on the next access anyway, right?
        Hide
        Lars Hofhansl added a comment -

        How about this one? This has still type-safety everywhere.

        Show
        Lars Hofhansl added a comment - How about this one? This has still type-safety everywhere.
        Hide
        Ted Yu added a comment -

        Addressed Lars' comments earlier.
        Also fixed HConstants.EMPTY_END_ROW in the comment which Lars suggested keeping.

        I didn't know Lars was working another patch.

        Show
        Ted Yu added a comment - Addressed Lars' comments earlier. Also fixed HConstants.EMPTY_END_ROW in the comment which Lars suggested keeping. I didn't know Lars was working another patch.
        Hide
        ronghai.ma added a comment -
        if (value.get() == null) {
          this.internalMap.remove(key);
          return null;
        }
        

        This is needed for splited regions.

        Show
        ronghai.ma added a comment - if (value.get() == null ) { this .internalMap.remove(key); return null ; } This is needed for splited regions.
        Hide
        Lars Hofhansl added a comment -

        @rongmai: How's that? This has nothing to do with split regions.

        Show
        Lars Hofhansl added a comment - @rongmai: How's that? This has nothing to do with split regions.
        Hide
        Ted Yu added a comment -

        Patch v6 adopts type-safety changes from Lars' patch.

        Show
        Ted Yu added a comment - Patch v6 adopts type-safety changes from Lars' patch.
        Hide
        Lars Hofhansl added a comment -

        @Ted: patch crossing

        Show
        Lars Hofhansl added a comment - @Ted: patch crossing
        Hide
        Ted Yu added a comment -

        @Lars:
        What do you think of patch v6 ?

        Show
        Ted Yu added a comment - @Lars: What do you think of patch v6 ?
        Hide
        Lars Hofhansl added a comment -

        +1 on v6
        Although I still think that

        +      if (value.get() == null) {
        +        this.internalMap.remove(key);
        +        return null;
        +      }
        

        is not needed.

        Show
        Lars Hofhansl added a comment - +1 on v6 Although I still think that + if (value.get() == null ) { + this .internalMap.remove(key); + return null ; + } is not needed.
        Hide
        ronghai.ma added a comment -

        yes, splited region will be deleted in locateRegionInMeta.

        Show
        ronghai.ma added a comment - yes, splited region will be deleted in locateRegionInMeta.
        Hide
        ronghai.ma added a comment -

        Without remove operation,
        are synchronized and checkRefrences() needed here?

        Show
        ronghai.ma added a comment - Without remove operation, are synchronized and checkRefrences() needed here?
        Hide
        Ted Yu added a comment -

        I prefer keeping the remove() call.
        Isn't it nice that stale reference gets cleared early ?

        Show
        Ted Yu added a comment - I prefer keeping the remove() call. Isn't it nice that stale reference gets cleared early ?
        Hide
        Lars Hofhansl added a comment -

        @ronghai: Null is returned either way, whether we remove it or not.

        Anyway, I guess we do the same in get(key), so let's leave it in.

        Show
        Lars Hofhansl added a comment - @ronghai: Null is returned either way, whether we remove it or not. Anyway, I guess we do the same in get(key), so let's leave it in.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12514420/5388-v6.txt
        against trunk revision .

        +1 @author. The patch does not contain any @author tags.

        -1 tests included. The patch doesn't appear to include any new or modified tests.
        Please justify why no new tests are needed for this patch.
        Also please list what manual steps were performed to verify this patch.

        -1 javadoc. The javadoc tool appears to have generated -136 warning messages.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        -1 findbugs. The patch appears to introduce 157 new Findbugs (version 1.3.9) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        -1 core tests. The patch failed these unit tests:
        org.apache.hadoop.hbase.mapreduce.TestImportTsv
        org.apache.hadoop.hbase.mapred.TestTableMapReduce
        org.apache.hadoop.hbase.mapreduce.TestHFileOutputFormat
        org.apache.hadoop.hbase.master.TestSplitLogManager

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/956//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/956//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/956//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12514420/5388-v6.txt against trunk revision . +1 @author. The patch does not contain any @author tags. -1 tests included. The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. -1 javadoc. The javadoc tool appears to have generated -136 warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 157 new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed these unit tests: org.apache.hadoop.hbase.mapreduce.TestImportTsv org.apache.hadoop.hbase.mapred.TestTableMapReduce org.apache.hadoop.hbase.mapreduce.TestHFileOutputFormat org.apache.hadoop.hbase.master.TestSplitLogManager Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/956//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/956//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/956//console This message is automatically generated.
        Hide
        Ted Yu added a comment -

        TestSplitLogManager passed locally on MacBook with patch v6.

        My script didn't find hanging test in build 956.

        Will integrate patch v6 tomorrow if there is no further review.

        Show
        Ted Yu added a comment - TestSplitLogManager passed locally on MacBook with patch v6. My script didn't find hanging test in build 956. Will integrate patch v6 tomorrow if there is no further review.
        Hide
        ronghai.ma added a comment -

        @Lars
        If we change(put,remove) internalMap, we use synchronized for thread safe.

        @Zhihong Yu
        Scenario: There is a large scale of regions cached(more than 120,000), and using HConnectionImplementation#processBatch with a list(more than 20) concurrently.

        Show
        ronghai.ma added a comment - @Lars If we change(put,remove) internalMap, we use synchronized for thread safe. @Zhihong Yu Scenario: There is a large scale of regions cached(more than 120,000), and using HConnectionImplementation#processBatch with a list(more than 20) concurrently.
        Hide
        Ted Yu added a comment -

        @Ronghai:
        Do you think patch v6 is good to go ?

        Thanks

        Show
        Ted Yu added a comment - @Ronghai: Do you think patch v6 is good to go ? Thanks
        Hide
        ronghai.ma added a comment -

        Excellent for this patch.
        Make another thread for the synchronized and checkReferences.

        Show
        ronghai.ma added a comment - Excellent for this patch. Make another thread for the synchronized and checkReferences.
        Hide
        Ted Yu added a comment -

        Integrated to TRUNK.

        Thanks for the patch Ronghai.

        Thanks for the review, Lars.

        @Ronghai:
        Please resolve after committer integrates patch to source repository

        Show
        Ted Yu added a comment - Integrated to TRUNK. Thanks for the patch Ronghai. Thanks for the review, Lars. @Ronghai: Please resolve after committer integrates patch to source repository
        Hide
        ronghai.ma added a comment -

        @Zhihong
        Thanks for the guide.

        @Lars
        Thanks for the review.

        Show
        ronghai.ma added a comment - @Zhihong Thanks for the guide. @Lars Thanks for the review.
        Hide
        Hudson added a comment -

        Integrated in HBase-TRUNK-security #112 (See https://builds.apache.org/job/HBase-TRUNK-security/112/)
        HBASE-5388 Tune HConnectionManager#getCachedLocation method (Ronghai Ma) (Revision 1243994)

        Result = SUCCESS
        tedyu :
        Files :

        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/util/SoftValueSortedMap.java
        Show
        Hudson added a comment - Integrated in HBase-TRUNK-security #112 (See https://builds.apache.org/job/HBase-TRUNK-security/112/ ) HBASE-5388 Tune HConnectionManager#getCachedLocation method (Ronghai Ma) (Revision 1243994) Result = SUCCESS tedyu : Files : /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/util/SoftValueSortedMap.java
        Hide
        Hudson added a comment -

        Integrated in HBase-TRUNK #2663 (See https://builds.apache.org/job/HBase-TRUNK/2663/)
        HBASE-5388 Tune HConnectionManager#getCachedLocation method (Ronghai Ma) (Revision 1243994)

        Result = FAILURE
        tedyu :
        Files :

        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/util/SoftValueSortedMap.java
        Show
        Hudson added a comment - Integrated in HBase-TRUNK #2663 (See https://builds.apache.org/job/HBase-TRUNK/2663/ ) HBASE-5388 Tune HConnectionManager#getCachedLocation method (Ronghai Ma) (Revision 1243994) Result = FAILURE tedyu : Files : /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/util/SoftValueSortedMap.java

          People

          • Assignee:
            ronghai.ma
            Reporter:
            ronghai.ma
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development