Hive
  1. Hive
  2. HIVE-3772

Fix a concurrency bug in LazyBinaryUtils due to a static field

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.9.0
    • Fix Version/s: 0.12.0
    • Labels:
      None

      Description

      Creating a JIRA for Reynold Xin's patch needed by the Shark project. https://github.com/amplab/hive/commit/17e1c3dd2f6d8eca767115dc46d5a880aed8c765
      writeVLong should not use a static field due to concurrency concerns.

      1. D7155.1.patch
        0.6 kB
        Phabricator
      2. HIVE-3772-2012-12-04.patch
        1 kB
        Mikhail Bautin
      3. D7155.2.patch
        1 kB
        Phabricator
      4. HIVE-3772.1.patch.txt
        1 kB
        Edward Capriolo

        Activity

        Hide
        Reynold Xin added a comment -

        Thanks for submitting this, Mikhail. Note that this was introduced in 0.9. In 0.7, this was not a problem ...

        Show
        Reynold Xin added a comment - Thanks for submitting this, Mikhail. Note that this was introduced in 0.9. In 0.7, this was not a problem ...
        Hide
        Phabricator added a comment -

        mbautin requested code review of "[jira] HIVE-3772 Fix a concurrency bug in LazyBinaryUtils due to a static field (patch by Reynold Xin)".
        Reviewers: ashutoshc, njain, raghotham, JIRA

        Reynold Xin's patch needed by the Shark project. https://github.com/amplab/hive/commit/17e1c3dd2f6d8eca767115dc46d5a880aed8c765
        (writeVLong should not use a static field due to concurrency concerns.)

        TEST PLAN
        Unit tests

        REVISION DETAIL
        https://reviews.facebook.net/D7155

        AFFECTED FILES
        serde/src/java/org/apache/hadoop/hive/serde2/lazybinary/LazyBinaryUtils.java

        MANAGE HERALD DIFFERENTIAL RULES
        https://reviews.facebook.net/herald/view/differential/

        WHY DID I GET THIS EMAIL?
        https://reviews.facebook.net/herald/transcript/16935/

        To: ashutoshc, njain, raghotham, JIRA, mbautin

        Show
        Phabricator added a comment - mbautin requested code review of " [jira] HIVE-3772 Fix a concurrency bug in LazyBinaryUtils due to a static field (patch by Reynold Xin)". Reviewers: ashutoshc, njain, raghotham, JIRA Reynold Xin's patch needed by the Shark project. https://github.com/amplab/hive/commit/17e1c3dd2f6d8eca767115dc46d5a880aed8c765 (writeVLong should not use a static field due to concurrency concerns.) TEST PLAN Unit tests REVISION DETAIL https://reviews.facebook.net/D7155 AFFECTED FILES serde/src/java/org/apache/hadoop/hive/serde2/lazybinary/LazyBinaryUtils.java MANAGE HERALD DIFFERENTIAL RULES https://reviews.facebook.net/herald/view/differential/ WHY DID I GET THIS EMAIL? https://reviews.facebook.net/herald/transcript/16935/ To: ashutoshc, njain, raghotham, JIRA, mbautin
        Hide
        Mikhail Bautin added a comment -

        Attaching trunk patch.

        Show
        Mikhail Bautin added a comment - Attaching trunk patch.
        Hide
        Phabricator added a comment -

        dhruba has resigned from the revision "[jira] HIVE-3772 Fix a concurrency bug in LazyBinaryUtils due to a static field (patch by Reynold Xin)".

        REVISION DETAIL
        https://reviews.facebook.net/D7155

        To: ashutoshc, njain, raghotham, JIRA, zshao, heyongqiang, nzhang, jsichi, pauly, amareshwarisr, cwsteinbach, mbautin

        Show
        Phabricator added a comment - dhruba has resigned from the revision " [jira] HIVE-3772 Fix a concurrency bug in LazyBinaryUtils due to a static field (patch by Reynold Xin)". REVISION DETAIL https://reviews.facebook.net/D7155 To: ashutoshc, njain, raghotham, JIRA, zshao, heyongqiang, nzhang, jsichi, pauly, amareshwarisr, cwsteinbach, mbautin
        Hide
        Phabricator added a comment -

        zshao has requested changes to the revision "[jira] HIVE-3772 Fix a concurrency bug in LazyBinaryUtils due to a static field (patch by Reynold Xin)".

        Please take this opportunity to fix all static variables in this file.

        By the way, a better way to fix is to use ThreadLocal. That's more effiicient because then we don't need to call new in these low-level functions.

        INLINE COMMENTS
        serde/src/java/org/apache/hadoop/hive/serde2/lazybinary/LazyBinaryUtils.java:126 Is this a problem for multi-threaded access as well?

        REVISION DETAIL
        https://reviews.facebook.net/D7155

        BRANCH
        HIVE-3772-static-vLongBytes

        To: ashutoshc, njain, raghotham, JIRA, zshao, heyongqiang, nzhang, jsichi, pauly, amareshwarisr, cwsteinbach, mbautin

        Show
        Phabricator added a comment - zshao has requested changes to the revision " [jira] HIVE-3772 Fix a concurrency bug in LazyBinaryUtils due to a static field (patch by Reynold Xin)". Please take this opportunity to fix all static variables in this file. By the way, a better way to fix is to use ThreadLocal. That's more effiicient because then we don't need to call new in these low-level functions. INLINE COMMENTS serde/src/java/org/apache/hadoop/hive/serde2/lazybinary/LazyBinaryUtils.java:126 Is this a problem for multi-threaded access as well? REVISION DETAIL https://reviews.facebook.net/D7155 BRANCH HIVE-3772 -static-vLongBytes To: ashutoshc, njain, raghotham, JIRA, zshao, heyongqiang, nzhang, jsichi, pauly, amareshwarisr, cwsteinbach, mbautin
        Hide
        Phabricator added a comment -

        mbautin updated the revision "[jira] HIVE-3772 Fix a concurrency bug in LazyBinaryUtils due to a static field (patch by Reynold Xin)".
        Reviewers: ashutoshc, njain, raghotham, JIRA, zshao, heyongqiang, nzhang, jsichi, pauly, amareshwarisr, cwsteinbach

        Addressing Zheng's comment by using ThreadLocal.

        REVISION DETAIL
        https://reviews.facebook.net/D7155

        AFFECTED FILES
        serde/src/java/org/apache/hadoop/hive/serde2/lazybinary/LazyBinaryUtils.java

        To: ashutoshc, njain, raghotham, JIRA, zshao, heyongqiang, nzhang, jsichi, pauly, amareshwarisr, cwsteinbach, mbautin

        Show
        Phabricator added a comment - mbautin updated the revision " [jira] HIVE-3772 Fix a concurrency bug in LazyBinaryUtils due to a static field (patch by Reynold Xin)". Reviewers: ashutoshc, njain, raghotham, JIRA, zshao, heyongqiang, nzhang, jsichi, pauly, amareshwarisr, cwsteinbach Addressing Zheng's comment by using ThreadLocal. REVISION DETAIL https://reviews.facebook.net/D7155 AFFECTED FILES serde/src/java/org/apache/hadoop/hive/serde2/lazybinary/LazyBinaryUtils.java To: ashutoshc, njain, raghotham, JIRA, zshao, heyongqiang, nzhang, jsichi, pauly, amareshwarisr, cwsteinbach, mbautin
        Hide
        Konstantin Boudnik added a comment -

        Is there any change of this get fixed in 0.11?

        Show
        Konstantin Boudnik added a comment - Is there any change of this get fixed in 0.11?
        Hide
        Konstantin Boudnik added a comment -

        The patch applies cleanly to the current 0.11 branch HEAD so it seems like a good candidate for maintenance 0.11.1 release?

        Show
        Konstantin Boudnik added a comment - The patch applies cleanly to the current 0.11 branch HEAD so it seems like a good candidate for maintenance 0.11.1 release?
        Hide
        Edward Capriolo added a comment -

        Looking now.

        Show
        Edward Capriolo added a comment - Looking now.
        Hide
        Edward Capriolo added a comment -

        I am +1. Thread local is not perfect but surely better then static.

        Show
        Edward Capriolo added a comment - I am +1. Thread local is not perfect but surely better then static.
        Hide
        Hive QA added a comment -

        Overall: -1 at least one tests failed

        Here are the results of testing the latest attachment:
        https://issues.apache.org/jira/secure/attachment/12597250/HIVE-3772.1.patch.txt

        ERROR: -1 due to 2 failed/errored test(s), 2774 tests executed
        Failed tests:

        org.apache.hcatalog.mapreduce.TestHCatExternalDynamicPartitioned.testHCatDynamicPartitionedTable
        org.apache.hadoop.hive.cli.TestNegativeCliDriver.testNegativeCliDriver_script_broken_pipe1
        

        Test results: https://builds.apache.org/job/PreCommit-HIVE-Build/381/testReport
        Console output: https://builds.apache.org/job/PreCommit-HIVE-Build/381/console

        Messages:

        Executing org.apache.hive.ptest.execution.PrepPhase
        Executing org.apache.hive.ptest.execution.ExecutionPhase
        Executing org.apache.hive.ptest.execution.ReportingPhase
        Tests failed with: TestsFailedException: 2 tests failed
        

        This message is automatically generated.

        Show
        Hive QA added a comment - Overall : -1 at least one tests failed Here are the results of testing the latest attachment: https://issues.apache.org/jira/secure/attachment/12597250/HIVE-3772.1.patch.txt ERROR: -1 due to 2 failed/errored test(s), 2774 tests executed Failed tests: org.apache.hcatalog.mapreduce.TestHCatExternalDynamicPartitioned.testHCatDynamicPartitionedTable org.apache.hadoop.hive.cli.TestNegativeCliDriver.testNegativeCliDriver_script_broken_pipe1 Test results: https://builds.apache.org/job/PreCommit-HIVE-Build/381/testReport Console output: https://builds.apache.org/job/PreCommit-HIVE-Build/381/console Messages: Executing org.apache.hive.ptest.execution.PrepPhase Executing org.apache.hive.ptest.execution.ExecutionPhase Executing org.apache.hive.ptest.execution.ReportingPhase Tests failed with: TestsFailedException: 2 tests failed This message is automatically generated.
        Hide
        Edward Capriolo added a comment -

        Committed. Thanks all.

        Show
        Edward Capriolo added a comment - Committed. Thanks all.
        Hide
        Konstantin Boudnik added a comment -

        Thank you so much, Edward.

        Show
        Konstantin Boudnik added a comment - Thank you so much, Edward.
        Hide
        Konstantin Boudnik added a comment -

        Edward, any chance it can be also backported into 0.11.1 ?

        Show
        Konstantin Boudnik added a comment - Edward, any chance it can be also backported into 0.11.1 ?
        Hide
        Hudson added a comment -

        FAILURE: Integrated in Hive-trunk-hadoop2-ptest #51 (See https://builds.apache.org/job/Hive-trunk-hadoop2-ptest/51/)
        HIVE-3772 Fix concurrency bug in LazyBinaryUtils due to a static field (Mikhail Bautin via egc)

        Submitted by:Mikhail Bautin and Reynold Xin
        Reviewed by: Edward Capriolo
        Approved by: Edward Capriolo (ecapriolo: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1512758)

        • /hive/trunk/serde/src/java/org/apache/hadoop/hive/serde2/lazybinary/LazyBinaryUtils.java
        Show
        Hudson added a comment - FAILURE: Integrated in Hive-trunk-hadoop2-ptest #51 (See https://builds.apache.org/job/Hive-trunk-hadoop2-ptest/51/ ) HIVE-3772 Fix concurrency bug in LazyBinaryUtils due to a static field (Mikhail Bautin via egc) Submitted by:Mikhail Bautin and Reynold Xin Reviewed by: Edward Capriolo Approved by: Edward Capriolo (ecapriolo: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1512758 ) /hive/trunk/serde/src/java/org/apache/hadoop/hive/serde2/lazybinary/LazyBinaryUtils.java
        Hide
        Edward Capriolo added a comment -

        Generally in hive we do not back port we just move forward. There are not many .1 or .2 releases.

        Show
        Edward Capriolo added a comment - Generally in hive we do not back port we just move forward. There are not many .1 or .2 releases.
        Hide
        Konstantin Boudnik added a comment -

        Got it, thanks for the explanation.

        Show
        Konstantin Boudnik added a comment - Got it, thanks for the explanation.
        Hide
        Hudson added a comment -

        SUCCESS: Integrated in Hive-trunk-hadoop1-ptest #122 (See https://builds.apache.org/job/Hive-trunk-hadoop1-ptest/122/)
        HIVE-3772 Fix concurrency bug in LazyBinaryUtils due to a static field (Mikhail Bautin via egc)

        Submitted by:Mikhail Bautin and Reynold Xin
        Reviewed by: Edward Capriolo
        Approved by: Edward Capriolo (ecapriolo: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1512758)

        • /hive/trunk/serde/src/java/org/apache/hadoop/hive/serde2/lazybinary/LazyBinaryUtils.java
        Show
        Hudson added a comment - SUCCESS: Integrated in Hive-trunk-hadoop1-ptest #122 (See https://builds.apache.org/job/Hive-trunk-hadoop1-ptest/122/ ) HIVE-3772 Fix concurrency bug in LazyBinaryUtils due to a static field (Mikhail Bautin via egc) Submitted by:Mikhail Bautin and Reynold Xin Reviewed by: Edward Capriolo Approved by: Edward Capriolo (ecapriolo: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1512758 ) /hive/trunk/serde/src/java/org/apache/hadoop/hive/serde2/lazybinary/LazyBinaryUtils.java
        Hide
        Hudson added a comment -

        ABORTED: Integrated in Hive-trunk-hadoop2 #350 (See https://builds.apache.org/job/Hive-trunk-hadoop2/350/)
        HIVE-3772 Fix concurrency bug in LazyBinaryUtils due to a static field (Mikhail Bautin via egc)

        Submitted by:Mikhail Bautin and Reynold Xin
        Reviewed by: Edward Capriolo
        Approved by: Edward Capriolo (ecapriolo: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1512758)

        • /hive/trunk/serde/src/java/org/apache/hadoop/hive/serde2/lazybinary/LazyBinaryUtils.java
        Show
        Hudson added a comment - ABORTED: Integrated in Hive-trunk-hadoop2 #350 (See https://builds.apache.org/job/Hive-trunk-hadoop2/350/ ) HIVE-3772 Fix concurrency bug in LazyBinaryUtils due to a static field (Mikhail Bautin via egc) Submitted by:Mikhail Bautin and Reynold Xin Reviewed by: Edward Capriolo Approved by: Edward Capriolo (ecapriolo: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1512758 ) /hive/trunk/serde/src/java/org/apache/hadoop/hive/serde2/lazybinary/LazyBinaryUtils.java
        Hide
        Hudson added a comment -

        SUCCESS: Integrated in Hive-trunk-h0.21 #2260 (See https://builds.apache.org/job/Hive-trunk-h0.21/2260/)
        HIVE-3772 Fix concurrency bug in LazyBinaryUtils due to a static field (Mikhail Bautin via egc)

        Submitted by:Mikhail Bautin and Reynold Xin
        Reviewed by: Edward Capriolo
        Approved by: Edward Capriolo (ecapriolo: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1512758)

        • /hive/trunk/serde/src/java/org/apache/hadoop/hive/serde2/lazybinary/LazyBinaryUtils.java
        Show
        Hudson added a comment - SUCCESS: Integrated in Hive-trunk-h0.21 #2260 (See https://builds.apache.org/job/Hive-trunk-h0.21/2260/ ) HIVE-3772 Fix concurrency bug in LazyBinaryUtils due to a static field (Mikhail Bautin via egc) Submitted by:Mikhail Bautin and Reynold Xin Reviewed by: Edward Capriolo Approved by: Edward Capriolo (ecapriolo: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1512758 ) /hive/trunk/serde/src/java/org/apache/hadoop/hive/serde2/lazybinary/LazyBinaryUtils.java
        Hide
        Ashutosh Chauhan added a comment -

        This issue has been fixed and released as part of 0.12 release. If you find further issues, please create a new jira and link it to this one.

        Show
        Ashutosh Chauhan added a comment - This issue has been fixed and released as part of 0.12 release. If you find further issues, please create a new jira and link it to this one.

          People

          • Assignee:
            Mikhail Bautin
            Reporter:
            Mikhail Bautin
          • Votes:
            2 Vote for this issue
            Watchers:
            7 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development