Details

    • Type: Bug
    • Status: Closed
    • Priority: Blocker
    • Resolution: Fixed
    • Affects Version/s: 0.9.0
    • Fix Version/s: 0.10.0
    • Component/s: Query Processor
    • Labels:
      None
    • Hadoop Flags:
      Reviewed
    • Release Note:
      Nullpointer on registering udfs.

      Description

      Currently the Function registry can throw NullPointers when multiple threads are trying to register the same function. The normal put() will replace the existing registered function object even if it's exactly the same function.

      1. HIVE-2544.1.patch.txt
        3 kB
        Bennie Schut
      2. HIVE-2544.patch.2.txt
        0.9 kB
        Edward Capriolo

        Issue Links

          Activity

          Hide
          bennies Bennie Schut added a comment -

          I'll add it to the review board tomorrow.

          Show
          bennies Bennie Schut added a comment - I'll add it to the review board tomorrow.
          Hide
          jiraposter@reviews.apache.org jiraposter@reviews.apache.org added a comment -

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          https://reviews.apache.org/r/2702/
          -----------------------------------------------------------

          Review request for hive.

          Summary
          -------

          HIVE-2544: Nullpointer on registering udfs.

          This addresses bug HIVE-2544.
          https://issues.apache.org/jira/browse/HIVE-2544

          Diffs


          trunk/ql/src/java/org/apache/hadoop/hive/ql/exec/FunctionRegistry.java 1196618

          Diff: https://reviews.apache.org/r/2702/diff

          Testing
          -------

          Thanks,

          Bennie

          Show
          jiraposter@reviews.apache.org jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2702/ ----------------------------------------------------------- Review request for hive. Summary ------- HIVE-2544 : Nullpointer on registering udfs. This addresses bug HIVE-2544 . https://issues.apache.org/jira/browse/HIVE-2544 Diffs trunk/ql/src/java/org/apache/hadoop/hive/ql/exec/FunctionRegistry.java 1196618 Diff: https://reviews.apache.org/r/2702/diff Testing ------- Thanks, Bennie
          Hide
          jiraposter@reviews.apache.org jiraposter@reviews.apache.org added a comment -

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          https://reviews.apache.org/r/2702/#review3927
          -----------------------------------------------------------

          trunk/ql/src/java/org/apache/hadoop/hive/ql/exec/FunctionRegistry.java
          <https://reviews.apache.org/r/2702/#comment8870>

          Is it possible to write a test case, which spins 10s of threads doing operations. Thats not fool-proof but will result in some coverage. Unless you have a better idea for writing test case for this.

          trunk/ql/src/java/org/apache/hadoop/hive/ql/exec/FunctionRegistry.java
          <https://reviews.apache.org/r/2702/#comment8869>

          Its LinkedHashMap right now, changing it to Concurrent will violate the contract of LinkedHashMap which you can iterate in same order as your inserts. Not sure if violating that is alright here.

          • Ashutosh

          On 2011-11-03 13:47:48, Bennie Schut wrote:

          -----------------------------------------------------------

          This is an automatically generated e-mail. To reply, visit:

          https://reviews.apache.org/r/2702/

          -----------------------------------------------------------

          (Updated 2011-11-03 13:47:48)

          Review request for hive.

          Summary

          -------

          HIVE-2544: Nullpointer on registering udfs.

          This addresses bug HIVE-2544.

          https://issues.apache.org/jira/browse/HIVE-2544

          Diffs

          -----

          trunk/ql/src/java/org/apache/hadoop/hive/ql/exec/FunctionRegistry.java 1196618

          Diff: https://reviews.apache.org/r/2702/diff

          Testing

          -------

          Thanks,

          Bennie

          Show
          jiraposter@reviews.apache.org jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2702/#review3927 ----------------------------------------------------------- trunk/ql/src/java/org/apache/hadoop/hive/ql/exec/FunctionRegistry.java < https://reviews.apache.org/r/2702/#comment8870 > Is it possible to write a test case, which spins 10s of threads doing operations. Thats not fool-proof but will result in some coverage. Unless you have a better idea for writing test case for this. trunk/ql/src/java/org/apache/hadoop/hive/ql/exec/FunctionRegistry.java < https://reviews.apache.org/r/2702/#comment8869 > Its LinkedHashMap right now, changing it to Concurrent will violate the contract of LinkedHashMap which you can iterate in same order as your inserts. Not sure if violating that is alright here. Ashutosh On 2011-11-03 13:47:48, Bennie Schut wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2702/ ----------------------------------------------------------- (Updated 2011-11-03 13:47:48) Review request for hive. Summary ------- HIVE-2544 : Nullpointer on registering udfs. This addresses bug HIVE-2544 . https://issues.apache.org/jira/browse/HIVE-2544 Diffs ----- trunk/ql/src/java/org/apache/hadoop/hive/ql/exec/FunctionRegistry.java 1196618 Diff: https://reviews.apache.org/r/2702/diff Testing ------- Thanks, Bennie
          Hide
          appodictic Edward Capriolo added a comment -

          s LinkedHashMap right now, changing it to Concurrent will violate the contract of LinkedHashMap which you can iterate in same order as your inserts. Not sure if violating that is ok here.

          From my prospective if it passes existing UNIT test the order does not matter.

          We could simply just do a concurrent map as well. I am +1 if tests pass. Any other comments?

          Show
          appodictic Edward Capriolo added a comment - s LinkedHashMap right now, changing it to Concurrent will violate the contract of LinkedHashMap which you can iterate in same order as your inserts. Not sure if violating that is ok here. From my prospective if it passes existing UNIT test the order does not matter. We could simply just do a concurrent map as well. I am +1 if tests pass. Any other comments?
          Hide
          ashutoshc Ashutosh Chauhan added a comment -

          Unlinking from 0.9

          Show
          ashutoshc Ashutosh Chauhan added a comment - Unlinking from 0.9
          Hide
          appodictic Edward Capriolo added a comment -

          I think the only time the order would matter is in show_functions.q. Lets revisit this.

          Show
          appodictic Edward Capriolo added a comment - I think the only time the order would matter is in show_functions.q. Lets revisit this.
          Hide
          appodictic Edward Capriolo added a comment -

          I have included a patch to use Collections.synchronizedMap() this should deal with atomic issues without changing the order of things.

          Show
          appodictic Edward Capriolo added a comment - I have included a patch to use Collections.synchronizedMap() this should deal with atomic issues without changing the order of things.
          Hide
          namit Namit Jain added a comment -

          @Edward, your patch solves the problem for registering UDFs.

          But, it is still possible to get NPEs, if different threads are performing showfunctions and register functions concurrently.
          Not sure, if you want to solve it in the same jira.
          Otherwise, please file a follow-up jira for that, and I will accept this one.

          Show
          namit Namit Jain added a comment - @Edward, your patch solves the problem for registering UDFs. But, it is still possible to get NPEs, if different threads are performing showfunctions and register functions concurrently. Not sure, if you want to solve it in the same jira. Otherwise, please file a follow-up jira for that, and I will accept this one.
          Hide
          namit Namit Jain added a comment -

          No code change.

          Cancelling the patch - once you create a new jira, please resubmit the patch

          Show
          namit Namit Jain added a comment - No code change. Cancelling the patch - once you create a new jira, please resubmit the patch
          Hide
          namit Namit Jain added a comment -

          I ran the tests - they ran fine.

          +1

          Show
          namit Namit Jain added a comment - I ran the tests - they ran fine. +1
          Hide
          namit Namit Jain added a comment -

          Committed. Thanks Edward

          Show
          namit Namit Jain added a comment - Committed. Thanks Edward
          Hide
          appodictic Edward Capriolo added a comment -

          Thanks Bennie too who did the hard work finding the issue.

          Show
          appodictic Edward Capriolo added a comment - Thanks Bennie too who did the hard work finding the issue.
          Hide
          hudson Hudson added a comment -

          Integrated in Hive-trunk-h0.21 #1548 (See https://builds.apache.org/job/Hive-trunk-h0.21/1548/)
          HIVE-2544 Nullpointer on registering udfs.
          (Edward Capriolo via namit) (Revision 1362374)

          Result = SUCCESS
          namit : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1362374
          Files :

          • /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/exec/FunctionRegistry.java
          Show
          hudson Hudson added a comment - Integrated in Hive-trunk-h0.21 #1548 (See https://builds.apache.org/job/Hive-trunk-h0.21/1548/ ) HIVE-2544 Nullpointer on registering udfs. (Edward Capriolo via namit) (Revision 1362374) Result = SUCCESS namit : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1362374 Files : /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/exec/FunctionRegistry.java
          Hide
          hudson Hudson added a comment -

          Integrated in Hive-trunk-hadoop2 #54 (See https://builds.apache.org/job/Hive-trunk-hadoop2/54/)
          HIVE-2544 Nullpointer on registering udfs.
          (Edward Capriolo via namit) (Revision 1362374)

          Result = ABORTED
          namit : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1362374
          Files :

          • /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/exec/FunctionRegistry.java
          Show
          hudson Hudson added a comment - Integrated in Hive-trunk-hadoop2 #54 (See https://builds.apache.org/job/Hive-trunk-hadoop2/54/ ) HIVE-2544 Nullpointer on registering udfs. (Edward Capriolo via namit) (Revision 1362374) Result = ABORTED namit : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1362374 Files : /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/exec/FunctionRegistry.java

            People

            • Assignee:
              appodictic Edward Capriolo
              Reporter:
              bennies Bennie Schut
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development