HBase
  1. HBase
  2. HBASE-5097

RegionObserver implementation whose preScannerOpen and postScannerOpen Impl return null can stall the system initialization through NPE

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.92.2, 0.94.0, 0.95.0
    • Component/s: Coprocessors
    • Labels:
      None

      Description

      In HRegionServer.java openScanner()

            r.prepareScanner(scan);
            RegionScanner s = null;
            if (r.getCoprocessorHost() != null) {
              s = r.getCoprocessorHost().preScannerOpen(scan);
            }
            if (s == null) {
              s = r.getScanner(scan);
            }
            if (r.getCoprocessorHost() != null) {
              s = r.getCoprocessorHost().postScannerOpen(scan, s);
            }
      

      If we dont have implemention for postScannerOpen the RegionScanner is null and so throwing nullpointer

      java.lang.NullPointerException
      	at java.util.concurrent.ConcurrentHashMap.put(ConcurrentHashMap.java:881)
      	at org.apache.hadoop.hbase.regionserver.HRegionServer.addScanner(HRegionServer.java:2282)
      	at org.apache.hadoop.hbase.regionserver.HRegionServer.openScanner(HRegionServer.java:2272)
      	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
      	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39)
      	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25)
      	at java.lang.reflect.Method.invoke(Method.java:597)
      	at org.apache.hadoop.hbase.ipc.WritableRpcEngine$Server.call(WritableRpcEngine.java:364)
      	at org.apache.hadoop.hbase.ipc.HBaseServer$Handler.run(HBaseServer.java:1326)
      
      

      Making this defect as blocker.. Pls feel free to change the priority if am wrong. Also correct me if my way of trying out coprocessors without implementing postScannerOpen is wrong. Am just a learner.

      1. HBASE-5097.patch
        1 kB
        ramkrishna.s.vasudevan
      2. HBASE-5097_1.patch
        0.8 kB
        ramkrishna.s.vasudevan
      3. HBASE-5097_2.patch
        0.9 kB
        ramkrishna.s.vasudevan

        Activity

        Hide
        Hudson added a comment -

        Integrated in HBase-0.92-security #104 (See https://builds.apache.org/job/HBase-0.92-security/104/)
        HBASE-5097 RegionObserver implementation whose preScannerOpen and postScannerOpen Impl return null can stall the system initialization through NPE (Ram) (Revision 1307034)

        Result = FAILURE
        ramkrishna :
        Files :

        • /hbase/branches/0.92/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
        Show
        Hudson added a comment - Integrated in HBase-0.92-security #104 (See https://builds.apache.org/job/HBase-0.92-security/104/ ) HBASE-5097 RegionObserver implementation whose preScannerOpen and postScannerOpen Impl return null can stall the system initialization through NPE (Ram) (Revision 1307034) Result = FAILURE ramkrishna : Files : /hbase/branches/0.92/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
        Hide
        Lars Hofhansl added a comment -

        To answer my own question: Yes

        Show
        Lars Hofhansl added a comment - To answer my own question: Yes
        Hide
        Lars Hofhansl added a comment -

        This can be closed, no?

        Show
        Lars Hofhansl added a comment - This can be closed, no?
        Hide
        Hudson added a comment -

        Integrated in HBase-TRUNK #2698 (See https://builds.apache.org/job/HBase-TRUNK/2698/)
        HBASE-5097 RegionObserver implementation whose preScannerOpen and postScannerOpen Impl return null can stall the system initialization through NPE (Ram) (Revision 1307036)

        Result = FAILURE
        ramkrishna :
        Files :

        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
        Show
        Hudson added a comment - Integrated in HBase-TRUNK #2698 (See https://builds.apache.org/job/HBase-TRUNK/2698/ ) HBASE-5097 RegionObserver implementation whose preScannerOpen and postScannerOpen Impl return null can stall the system initialization through NPE (Ram) (Revision 1307036) Result = FAILURE ramkrishna : Files : /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
        Hide
        Hudson added a comment -

        Integrated in HBase-0.94-security #6 (See https://builds.apache.org/job/HBase-0.94-security/6/)
        HBASE-5097 RegionObserver implementation whose preScannerOpen and postScannerOpen Impl return null can stall the system initialization through NPE (Ram) (Revision 1307035)

        Result = SUCCESS
        ramkrishna :
        Files :

        • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
        Show
        Hudson added a comment - Integrated in HBase-0.94-security #6 (See https://builds.apache.org/job/HBase-0.94-security/6/ ) HBASE-5097 RegionObserver implementation whose preScannerOpen and postScannerOpen Impl return null can stall the system initialization through NPE (Ram) (Revision 1307035) Result = SUCCESS ramkrishna : Files : /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
        Hide
        Hudson added a comment -

        Integrated in HBase-TRUNK-security #154 (See https://builds.apache.org/job/HBase-TRUNK-security/154/)
        HBASE-5097 RegionObserver implementation whose preScannerOpen and postScannerOpen Impl return null can stall the system initialization through NPE (Ram) (Revision 1307036)

        Result = FAILURE
        ramkrishna :
        Files :

        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
        Show
        Hudson added a comment - Integrated in HBase-TRUNK-security #154 (See https://builds.apache.org/job/HBase-TRUNK-security/154/ ) HBASE-5097 RegionObserver implementation whose preScannerOpen and postScannerOpen Impl return null can stall the system initialization through NPE (Ram) (Revision 1307036) Result = FAILURE ramkrishna : Files : /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
        Hide
        Hudson added a comment -

        Integrated in HBase-0.92 #342 (See https://builds.apache.org/job/HBase-0.92/342/)
        HBASE-5097 RegionObserver implementation whose preScannerOpen and postScannerOpen Impl return null can stall the system initialization through NPE (Ram) (Revision 1307034)

        Result = SUCCESS
        ramkrishna :
        Files :

        • /hbase/branches/0.92/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
        Show
        Hudson added a comment - Integrated in HBase-0.92 #342 (See https://builds.apache.org/job/HBase-0.92/342/ ) HBASE-5097 RegionObserver implementation whose preScannerOpen and postScannerOpen Impl return null can stall the system initialization through NPE (Ram) (Revision 1307034) Result = SUCCESS ramkrishna : Files : /hbase/branches/0.92/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
        Hide
        Hudson added a comment -

        Integrated in HBase-0.94 #65 (See https://builds.apache.org/job/HBase-0.94/65/)
        HBASE-5097 RegionObserver implementation whose preScannerOpen and postScannerOpen Impl return null can stall the system initialization through NPE (Ram) (Revision 1307035)

        Result = SUCCESS
        ramkrishna :
        Files :

        • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
        Show
        Hudson added a comment - Integrated in HBase-0.94 #65 (See https://builds.apache.org/job/HBase-0.94/65/ ) HBASE-5097 RegionObserver implementation whose preScannerOpen and postScannerOpen Impl return null can stall the system initialization through NPE (Ram) (Revision 1307035) Result = SUCCESS ramkrishna : Files : /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
        Hide
        ramkrishna.s.vasudevan added a comment -

        Committed to 0.94, 0.92 and trunk. Sorry for the delay in committing the patches.
        Thanks to Stack, Andy, Lars and Ted for the review.

        Show
        ramkrishna.s.vasudevan added a comment - Committed to 0.94, 0.92 and trunk. Sorry for the delay in committing the patches. Thanks to Stack, Andy, Lars and Ted for the review.
        Hide
        Lars Hofhansl added a comment -

        Feel free to put it in 0.94.0. Will cut an RC today.

        Show
        Lars Hofhansl added a comment - Feel free to put it in 0.94.0. Will cut an RC today.
        Hide
        ramkrishna.s.vasudevan added a comment -

        This patch is not committed yet.!!
        I will do that today..

        Show
        ramkrishna.s.vasudevan added a comment - This patch is not committed yet.!! I will do that today..
        Hide
        stack added a comment -

        +1 on patch

        Show
        stack added a comment - +1 on patch
        Hide
        Andrew Purtell added a comment -

        +1 on fixed patch.

        Show
        Andrew Purtell added a comment - +1 on fixed patch.
        Hide
        ramkrishna.s.vasudevan added a comment -

        Indention problem occured by mistake. Corrected patch uploaded.

        Show
        ramkrishna.s.vasudevan added a comment - Indention problem occured by mistake. Corrected patch uploaded.
        Hide
        Lars Hofhansl added a comment -

        +1 on patch (with fixed indentation ). Test failures must be unrelated.

        Show
        Lars Hofhansl added a comment - +1 on patch (with fixed indentation ). Test failures must be unrelated.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12509324/HBASE-5097_1.patch
        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 -151 warning messages.

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

        -1 findbugs. The patch appears to introduce 77 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

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/653//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/653//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/653//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/12509324/HBASE-5097_1.patch 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 -151 warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 77 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 Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/653//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/653//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/653//console This message is automatically generated.
        Hide
        Ted Yu added a comment -

        The indentation is off. Should have used two spaces.

        Otherwise patch looks good.

        Show
        Ted Yu added a comment - The indentation is off. Should have used two spaces. Otherwise patch looks good.
        Hide
        ramkrishna.s.vasudevan added a comment -

        Addressed comments from Lars and Ted.

        Show
        ramkrishna.s.vasudevan added a comment - Addressed comments from Lars and Ted.
        Hide
        ramkrishna.s.vasudevan added a comment -

        @Lars and @Ted
        Will submit a patch soon

        Show
        ramkrishna.s.vasudevan added a comment - @Lars and @Ted Will submit a patch soon
        Hide
        Ted Yu added a comment -

        I think Lars' idea is fine.
        Naming temp 'savedScanner' should be in next patch.

        Show
        Ted Yu added a comment - I think Lars' idea is fine. Naming temp 'savedScanner' should be in next patch.
        Hide
        Lars Hofhansl added a comment -

        @Ted and @Ram... What do you think of my idea above?

        Show
        Lars Hofhansl added a comment - @Ted and @Ram... What do you think of my idea above?
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12508902/HBASE-5097.patch
        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 -151 warning messages.

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

        -1 findbugs. The patch appears to introduce 76 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 passed unit tests in .

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/637//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/637//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/637//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/12508902/HBASE-5097.patch 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 -151 warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 76 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 passed unit tests in . Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/637//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/637//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/637//console This message is automatically generated.
        Hide
        Ted Yu added a comment -
               if (s == null) {
                 s = r.getScanner(scan);
        +        temp = s;
               }
        

        How about naming temp 'savedScanner' ?

        I think the assignment should be outside the above if block:

               if (s == null) {
                 s = r.getScanner(scan);
               }
        +      savedScanner = s;
        
        Show
        Ted Yu added a comment - if (s == null ) { s = r.getScanner(scan); + temp = s; } How about naming temp 'savedScanner' ? I think the assignment should be outside the above if block: if (s == null ) { s = r.getScanner(scan); } + savedScanner = s;
        Hide
        Lars Hofhansl added a comment -

        Looks good. How about:

        ...
        if (r.getCoprocessorHost() != null) {
          RegionScanner temp = r.getCoprocessorHost().postScannerOpen(scan, s);
          if (temp == null) {
            LOG.warn("PostScannerOpen impl returning null. Check the RegionObserver implementation.");
          } else {
            s = temp;
          }
        }
        ...
        

        That way the temporary RegionScanner is local to the postScanner check making it (IMHO) easier to read.

        Show
        Lars Hofhansl added a comment - Looks good. How about: ... if (r.getCoprocessorHost() != null ) { RegionScanner temp = r.getCoprocessorHost().postScannerOpen(scan, s); if (temp == null ) { LOG.warn( "PostScannerOpen impl returning null . Check the RegionObserver implementation." ); } else { s = temp; } } ... That way the temporary RegionScanner is local to the postScanner check making it (IMHO) easier to read.
        Hide
        ramkrishna.s.vasudevan added a comment -

        Trunk patch

        Show
        ramkrishna.s.vasudevan added a comment - Trunk patch
        Hide
        Andrew Purtell added a comment -

        We should save the non-null s before calling postScannerOpen(). If postScannerOpen() returns null, we can use the saved scanner.

        ... and emit a warning. Sounds good.

        Show
        Andrew Purtell added a comment - We should save the non-null s before calling postScannerOpen(). If postScannerOpen() returns null, we can use the saved scanner. ... and emit a warning. Sounds good.
        Hide
        Ted Yu added a comment -

        We should save the non-null s before calling postScannerOpen().
        If postScannerOpen() returns null, we can use the saved scanner.

        Show
        Ted Yu added a comment - We should save the non-null s before calling postScannerOpen(). If postScannerOpen() returns null, we can use the saved scanner.
        Hide
        Lars Hofhansl added a comment - - edited

        fair enough. So if postScannerOpen() returns null we just won't create a scanner?
        Might lead to hard to detect problems.

        Show
        Lars Hofhansl added a comment - - edited fair enough. So if postScannerOpen() returns null we just won't create a scanner? Might lead to hard to detect problems.
        Hide
        Ted Yu added a comment -

        I think the case here is that we should check for s not being null before calling addScanner(s).
        Some user may deliberately write code where preScannerOpen and postScannerOpen implementations return null to stall initialization.

        Eugene implemented feature to take off bad-behaving coprocessors through hbase.coprocessor.abortonerror config parameter (see CoprocessorHost.handleCoprocessorThrowable).
        I think we should cover this case as well.

        Show
        Ted Yu added a comment - I think the case here is that we should check for s not being null before calling addScanner(s). Some user may deliberately write code where preScannerOpen and postScannerOpen implementations return null to stall initialization. Eugene implemented feature to take off bad-behaving coprocessors through hbase.coprocessor.abortonerror config parameter (see CoprocessorHost.handleCoprocessorThrowable). I think we should cover this case as well.
        Hide
        ramkrishna.s.vasudevan added a comment -

        @Lars..

        Ok Lars..I too accept it..

        Show
        ramkrishna.s.vasudevan added a comment - @Lars.. Ok Lars..I too accept it..
        Hide
        Lars Hofhansl added a comment -

        Hmmm. There is no default implementation for an interface. Are you referring to the default implementation generated for you by eclipse?
        I just don't think this is a bug.

        Show
        Lars Hofhansl added a comment - Hmmm. There is no default implementation for an interface. Are you referring to the default implementation generated for you by eclipse? I just don't think this is a bug.
        Hide
        ramkrishna.s.vasudevan added a comment -

        @Lars

        I implemented RegionObserver and not BaseRegionObserver. In BaseRegionObserver we return a regionscanner so it will not cause any problem. But if we implement RegionObserver then default will be null value. That is the problem.

        Show
        ramkrishna.s.vasudevan added a comment - @Lars I implemented RegionObserver and not BaseRegionObserver. In BaseRegionObserver we return a regionscanner so it will not cause any problem. But if we implement RegionObserver then default will be null value. That is the problem.
        Hide
        Lars Hofhansl added a comment -

        Not a big fan of catching NPEs. We can add a null check somewhere in the code, but catching NPEs is bad design (IMHO).

        @Ram: How did you actually do this? You need to implement RegionObserver, so you cannot compile unless you provide an implementation of postScannerOpen. Or did you class not even implement RegionObserver?

        Show
        Lars Hofhansl added a comment - Not a big fan of catching NPEs. We can add a null check somewhere in the code, but catching NPEs is bad design (IMHO). @Ram: How did you actually do this? You need to implement RegionObserver, so you cannot compile unless you provide an implementation of postScannerOpen. Or did you class not even implement RegionObserver?
        Hide
        Andrew Purtell added a comment -

        Wouldn't hurt.

        Show
        Andrew Purtell added a comment - Wouldn't hurt.
        Hide
        Ted Yu added a comment -

        Should we catch the NullPointer exception and provide better message ?

        Show
        Ted Yu added a comment - Should we catch the NullPointer exception and provide better message ?
        Hide
        ramkrishna.s.vasudevan added a comment -

        Ya.. My bad....

        Show
        ramkrishna.s.vasudevan added a comment - Ya.. My bad.. ..
        Hide
        Andrew Purtell added a comment -

        Are you inheriting from BaseRegionObserver? I'd guess not?

        Show
        Andrew Purtell added a comment - Are you inheriting from BaseRegionObserver ? I'd guess not?
        Hide
        ramkrishna.s.vasudevan added a comment -

        Changing the priority.

        Show
        ramkrishna.s.vasudevan added a comment - Changing the priority.

          People

          • Assignee:
            ramkrishna.s.vasudevan
            Reporter:
            ramkrishna.s.vasudevan
          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development