HBase
  1. HBase
  2. HBASE-5532

get NPE during MajorCompactionChecker

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Won't Fix
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: regionserver
    • Labels:

      Description

      We found error log (NullPointerException) below on our online cluster:

      2012-03-05 00:17:09,592 ERROR org.apache.hadoop.hbase.regionserver.HRegionServer$MajorCompactionChecker: Caught exception
      java.lang.NullPointerException
      at org.apache.hadoop.hbase.regionserver.Store.isMajorCompaction(Store.java:878)
      at org.apache.hadoop.hbase.regionserver.Store.isMajorCompaction(Store.java:857)
      at org.apache.hadoop.hbase.regionserver.HRegion.isMajorCompaction(HRegion.java:3017)
      at org.apache.hadoop.hbase.regionserver.HRegionServer$MajorCompactionChecker.chore(HRegionServer.java:1172)
      at org.apache.hadoop.hbase.Chore.run(Chore.java:66)

      After Check the code we found although it already check whether store files has null reader at the begin of the function(isMajorCompaction), but it still has some possibility the reader is closed before it return(eg mini compaction). So we need to check store file reader before we use it to avoid this NPE

      1. HBASE-5532.patch
        0.7 kB
        terry zhang
      2. HBASE-5532-v2.patch
        1 kB
        terry zhang

        Activity

        Hide
        Ted Yu added a comment -

        Which version of HBase were you using ?

        Show
        Ted Yu added a comment - Which version of HBase were you using ?
        Hide
        terry zhang added a comment -

        Hi, Ted. Our is base on 0.90.2 (including many patches from 90.3 till 90.5).

        Show
        terry zhang added a comment - Hi, Ted. Our is base on 0.90.2 (including many patches from 90.3 till 90.5).
        Hide
        Nicolas Spiegelberg added a comment -

        Sounds like this is a race condition, so this patch wouldn't be sufficient. We need some way to lock the reader during this query.

        Show
        Nicolas Spiegelberg added a comment - Sounds like this is a race condition, so this patch wouldn't be sufficient. We need some way to lock the reader during this query.
        Hide
        terry zhang added a comment -

        yes,Nicolas. This is a race condition and we can use lock to avoid this issue. And now in the function toDetermines if Store should be split also didn't use lock to protect.

        Store.java
          /**
           * Determines if Store should be split
           * @return byte[] if store should be split, null otherwise.
           */
          public byte[] getSplitPoint() {
            this.lock.readLock().lock();
            try {
              // sanity checks
              if (this.storefiles.isEmpty()) {
                return null;
              }
         
        
         
        
                StoreFile.Reader r = sf.getReader(); * <= check if the reader is null*
                if (r == null) {
                  LOG.warn("Storefile " + sf + " Reader is null");
                  continue;
                }
        
                long size = r.length();
                if (size > maxSize) {
                  // This is the largest one so far
                  maxSize = size;
                  largestSf = sf;
                }
              }
        
              StoreFile.Reader r = largestSf.getReader(); *<= check if the reader is null*
              if (r == null) {
                LOG.warn("Storefile " + largestSf + " Reader is null");
                return null;
              }
              // Get first, last, and mid keys.  Midkey is the key that starts block
              // in middle of hfile.  Has column and timestamp.  Need to return just
              // the row we want to split on as midkey.
              byte [] midkey = r.midkey();
             .....
            return null;
          }
        
        Show
        terry zhang added a comment - yes,Nicolas. This is a race condition and we can use lock to avoid this issue. And now in the function toDetermines if Store should be split also didn't use lock to protect. Store.java /** * Determines if Store should be split * @ return byte [] if store should be split, null otherwise. */ public byte [] getSplitPoint() { this .lock.readLock().lock(); try { // sanity checks if ( this .storefiles.isEmpty()) { return null ; } StoreFile.Reader r = sf.getReader(); * <= check if the reader is null * if (r == null ) { LOG.warn( "Storefile " + sf + " Reader is null " ); continue ; } long size = r.length(); if (size > maxSize) { // This is the largest one so far maxSize = size; largestSf = sf; } } StoreFile.Reader r = largestSf.getReader(); *<= check if the reader is null * if (r == null ) { LOG.warn( "Storefile " + largestSf + " Reader is null " ); return null ; } // Get first, last, and mid keys. Midkey is the key that starts block // in middle of hfile. Has column and timestamp. Need to return just // the row we want to split on as midkey. byte [] midkey = r.midkey(); ..... return null ; }
        Hide
        terry zhang added a comment -

        add a read lock during isMajorCompaction() to avoid race condition.

        Show
        terry zhang added a comment - add a read lock during isMajorCompaction() to avoid race condition.
        Hide
        stack added a comment -

        This looks like patch for 0.90 only?

        Show
        stack added a comment - This looks like patch for 0.90 only?
        Hide
        Cosmin Lehene added a comment -

        terry zhang, Andrew Purtell this may not apply anymore.
        The code has changed a bit, and it seems we check for null readers. I'm not sure about the locking semantics.

        The logic has moved to StoreFile.java and there's related logic in StripeStoreFileManager
        isMajorCompaction() is in HStore.java

        Show
        Cosmin Lehene added a comment - terry zhang , Andrew Purtell this may not apply anymore. The code has changed a bit, and it seems we check for null readers. I'm not sure about the locking semantics. The logic has moved to StoreFile.java and there's related logic in StripeStoreFileManager isMajorCompaction() is in HStore.java
        Hide
        Andrew Purtell added a comment -

        Thanks for looking at this Cosmin Lehene. As this was reported against 0.90 and we don't support that version now I'm closing as Wont Fix

        Show
        Andrew Purtell added a comment - Thanks for looking at this Cosmin Lehene . As this was reported against 0.90 and we don't support that version now I'm closing as Wont Fix

          People

          • Assignee:
            Unassigned
            Reporter:
            terry zhang
          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development