Hive
  1. Hive
  2. HIVE-2833

Fix test failures caused by HIVE-2716

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.9.0
    • Component/s: Metastore
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Issue Links

        Activity

        Hide
        Carl Steinbach added a comment -

        @Ashutosh, it broke facebook's internal branch.
        Did you test it with mysql db - I haven't debugged it, but reverting this fixes the problem.

        Show
        Carl Steinbach added a comment - @Ashutosh, it broke facebook's internal branch. Did you test it with mysql db - I haven't debugged it, but reverting this fixes the problem.
        Hide
        Namit Jain added a comment -

        Can you please work on this as a very high priority ?
        We need a fix asap for this.

        Show
        Namit Jain added a comment - Can you please work on this as a very high priority ? We need a fix asap for this.
        Hide
        Ashutosh Chauhan added a comment -

        @Namit,

        It passes all the tests on the trunk. Do, you have a reproducible testcase for this?

        Show
        Ashutosh Chauhan added a comment - @Namit, It passes all the tests on the trunk. Do, you have a reproducible testcase for this?
        Hide
        Enis Soztutar added a comment -

        I can work on it, if we can get a reproducible test, or a list of tests which are failing.

        Is there an easy way to run the tests on trunk with mysql?

        Show
        Enis Soztutar added a comment - I can work on it, if we can get a reproducible test, or a list of tests which are failing. Is there an easy way to run the tests on trunk with mysql?
        Hide
        Carl Steinbach added a comment -

        @Enis: Modifying the metastore configuration properties in data/conf/hive-site.xml will probably work for most tests.

        Show
        Carl Steinbach added a comment - @Enis: Modifying the metastore configuration properties in data/conf/hive-site.xml will probably work for most tests.
        Hide
        Namit Jain added a comment -

        Enis/Ashutosh, we can try coming up with the testcase.
        But, in the meanwhile, can I upload a patch which reverts this ?
        This will unblock us (facebook branch).

        Show
        Namit Jain added a comment - Enis/Ashutosh, we can try coming up with the testcase. But, in the meanwhile, can I upload a patch which reverts this ? This will unblock us (facebook branch).
        Hide
        Kevin Wilfong added a comment -

        The issue is that updateConnectionURL is now being called after the ObjectStore is initially constructed. We use a Metastore Connection URL Hook to generate the URL to be used by JDO. Since, this was being called after the ObjectStore was constructed, it was initially trying to connect to a default URL which was incorrect.

        I'll upload a patch once I come up with a test case.

        Show
        Kevin Wilfong added a comment - The issue is that updateConnectionURL is now being called after the ObjectStore is initially constructed. We use a Metastore Connection URL Hook to generate the URL to be used by JDO. Since, this was being called after the ObjectStore was constructed, it was initially trying to connect to a default URL which was incorrect. I'll upload a patch once I come up with a test case.
        Hide
        Kevin Wilfong added a comment -

        kevinwilfong requested code review of "HIVE-2716 [jira] Move retry logic in HiveMetaStore to a separe class".
        Reviewers: JIRA

        This fixes the issue where the implementation of RawStore was being initialized in the metastore before the JDOConnectionURLHook was being called. I moved some code around so that the RawStore implementation is initialized in the RetryRawStore initialization after the hook is called. In addition, the URL now needs to be updated in the thread local configuration instead of the non-thread local one, as by the time the hook is now being called, the thread local configuration has already been initialized based on the other.

        In HIVE-1219, method retrying for raw store operation are introduced to handle jdo operations more robustly. However, the abstraction for the RawStore operations can be moved to a separate class implementing RawStore, which should clean up the code base for HiveMetaStore.

        TEST PLAN
        EMPTY

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

        AFFECTED FILES
        metastore/src/test/org/apache/hadoop/hive/metastore/DummyRawStoreForJdoConnection.java
        metastore/src/test/org/apache/hadoop/hive/metastore/TestMetaStoreConnectionUrlHook.java
        metastore/src/test/org/apache/hadoop/hive/metastore/DummyJdoConnectionUrlHook.java
        metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
        metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreUtils.java
        metastore/src/java/org/apache/hadoop/hive/metastore/RetryingRawStore.java

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

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

        Tip: use the X-Herald-Rules header to filter Herald messages in your client.

        Show
        Kevin Wilfong added a comment - kevinwilfong requested code review of " HIVE-2716 [jira] Move retry logic in HiveMetaStore to a separe class". Reviewers: JIRA This fixes the issue where the implementation of RawStore was being initialized in the metastore before the JDOConnectionURLHook was being called. I moved some code around so that the RawStore implementation is initialized in the RetryRawStore initialization after the hook is called. In addition, the URL now needs to be updated in the thread local configuration instead of the non-thread local one, as by the time the hook is now being called, the thread local configuration has already been initialized based on the other. In HIVE-1219 , method retrying for raw store operation are introduced to handle jdo operations more robustly. However, the abstraction for the RawStore operations can be moved to a separate class implementing RawStore, which should clean up the code base for HiveMetaStore. TEST PLAN EMPTY REVISION DETAIL https://reviews.facebook.net/D2055 AFFECTED FILES metastore/src/test/org/apache/hadoop/hive/metastore/DummyRawStoreForJdoConnection.java metastore/src/test/org/apache/hadoop/hive/metastore/TestMetaStoreConnectionUrlHook.java metastore/src/test/org/apache/hadoop/hive/metastore/DummyJdoConnectionUrlHook.java metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreUtils.java metastore/src/java/org/apache/hadoop/hive/metastore/RetryingRawStore.java MANAGE HERALD DIFFERENTIAL RULES https://reviews.facebook.net/herald/view/differential/ WHY DID I GET THIS EMAIL? https://reviews.facebook.net/herald/transcript/4407/ Tip: use the X-Herald-Rules header to filter Herald messages in your client.
        Hide
        Phabricator added a comment -

        enis has commented on the revision "HIVE-2833 [jira] Fix test failures caused by HIVE-2716".

        INLINE COMMENTS
        metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreUtils.java:1019 What is class1 is used for? I noticed that you did not change it but just moved the method. Shall we remove that parameter.
        metastore/src/test/org/apache/hadoop/hive/metastore/TestMetaStoreConnectionUrlHook.java:49 Changing this to DummyJdoConnectionUrlHook.class.getName() will be better for refactoring / finding references.
        metastore/src/test/org/apache/hadoop/hive/metastore/TestMetaStoreConnectionUrlHook.java:53 same here
        metastore/src/test/org/apache/hadoop/hive/metastore/TestMetaStoreConnectionUrlHook.java:32 There is a separate test for the metastorehook in
        contrib/src/test/queries/clientnegative/url_hook.q and
        contrib/src/java/org/apache/hadoop/hive/contrib/metastore/hooks/TestURLHook.java, which tests the meta store hook connection retry mechanism. Shall we also migrate that test here, wdyt?
        Please see that patch at https://reviews.facebook.net/D1953
        metastore/src/test/org/apache/hadoop/hive/metastore/DummyJdoConnectionUrlHook.java:33 Can you remove this, it is not used.

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

        Show
        Phabricator added a comment - enis has commented on the revision " HIVE-2833 [jira] Fix test failures caused by HIVE-2716 ". INLINE COMMENTS metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreUtils.java:1019 What is class1 is used for? I noticed that you did not change it but just moved the method. Shall we remove that parameter. metastore/src/test/org/apache/hadoop/hive/metastore/TestMetaStoreConnectionUrlHook.java:49 Changing this to DummyJdoConnectionUrlHook.class.getName() will be better for refactoring / finding references. metastore/src/test/org/apache/hadoop/hive/metastore/TestMetaStoreConnectionUrlHook.java:53 same here metastore/src/test/org/apache/hadoop/hive/metastore/TestMetaStoreConnectionUrlHook.java:32 There is a separate test for the metastorehook in contrib/src/test/queries/clientnegative/url_hook.q and contrib/src/java/org/apache/hadoop/hive/contrib/metastore/hooks/TestURLHook.java, which tests the meta store hook connection retry mechanism. Shall we also migrate that test here, wdyt? Please see that patch at https://reviews.facebook.net/D1953 metastore/src/test/org/apache/hadoop/hive/metastore/DummyJdoConnectionUrlHook.java:33 Can you remove this, it is not used. REVISION DETAIL https://reviews.facebook.net/D2055
        Hide
        Phabricator added a comment -

        kevinwilfong has commented on the revision "HIVE-2833 [jira] Fix test failures caused by HIVE-2716".

        INLINE COMMENTS
        metastore/src/test/org/apache/hadoop/hive/metastore/DummyJdoConnectionUrlHook.java:33 It is used in the test method. I thought it made sense to keep them together.
        metastore/src/test/org/apache/hadoop/hive/metastore/TestMetaStoreConnectionUrlHook.java:32 A q file will test the entire stack from CLI to the db underlying the Object Store, so it is much more complete. In this case, I just wanted to target a very specific test.

        Also, I don't like to change tests other people have written unless there is a clear problem with the test. In case I miss something they wanted to test.

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

        Show
        Phabricator added a comment - kevinwilfong has commented on the revision " HIVE-2833 [jira] Fix test failures caused by HIVE-2716 ". INLINE COMMENTS metastore/src/test/org/apache/hadoop/hive/metastore/DummyJdoConnectionUrlHook.java:33 It is used in the test method. I thought it made sense to keep them together. metastore/src/test/org/apache/hadoop/hive/metastore/TestMetaStoreConnectionUrlHook.java:32 A q file will test the entire stack from CLI to the db underlying the Object Store, so it is much more complete. In this case, I just wanted to target a very specific test. Also, I don't like to change tests other people have written unless there is a clear problem with the test. In case I miss something they wanted to test. REVISION DETAIL https://reviews.facebook.net/D2055
        Hide
        Phabricator added a comment -

        kevinwilfong updated the revision "HIVE-2833 [jira] Fix test failures caused by HIVE-2716".
        Reviewers: JIRA, njain, ashutoshc, cwsteinbach, enis

        Removed unnecessary parameter from getClass, used getName method of classes instead of explicit class names in test.

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

        AFFECTED FILES
        metastore/src/test/org/apache/hadoop/hive/metastore/DummyRawStoreForJdoConnection.java
        metastore/src/test/org/apache/hadoop/hive/metastore/TestMetaStoreConnectionUrlHook.java
        metastore/src/test/org/apache/hadoop/hive/metastore/DummyJdoConnectionUrlHook.java
        metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
        metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreUtils.java
        metastore/src/java/org/apache/hadoop/hive/metastore/RetryingRawStore.java

        Show
        Phabricator added a comment - kevinwilfong updated the revision " HIVE-2833 [jira] Fix test failures caused by HIVE-2716 ". Reviewers: JIRA, njain, ashutoshc, cwsteinbach, enis Removed unnecessary parameter from getClass, used getName method of classes instead of explicit class names in test. REVISION DETAIL https://reviews.facebook.net/D2055 AFFECTED FILES metastore/src/test/org/apache/hadoop/hive/metastore/DummyRawStoreForJdoConnection.java metastore/src/test/org/apache/hadoop/hive/metastore/TestMetaStoreConnectionUrlHook.java metastore/src/test/org/apache/hadoop/hive/metastore/DummyJdoConnectionUrlHook.java metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreUtils.java metastore/src/java/org/apache/hadoop/hive/metastore/RetryingRawStore.java
        Hide
        Namit Jain added a comment -

        Looks good to me, but I will wait from Ashutosh/Enis before I commit this.
        Please let us know if you see any problems with this.

        Show
        Namit Jain added a comment - Looks good to me, but I will wait from Ashutosh/Enis before I commit this. Please let us know if you see any problems with this.
        Hide
        Ashutosh Chauhan added a comment -

        @Kevin,
        Thanks a bunch for digging into it and for the fix.

        @Namit,
        I will take a look at it over the weekend and will post comments if I have any.

        Show
        Ashutosh Chauhan added a comment - @Kevin, Thanks a bunch for digging into it and for the fix. @Namit, I will take a look at it over the weekend and will post comments if I have any.
        Hide
        Phabricator added a comment -

        ashutoshc has accepted the revision "HIVE-2833 [jira] Fix test failures caused by HIVE-2716".

        +1 Looks good to me.

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

        BRANCH
        svn

        Show
        Phabricator added a comment - ashutoshc has accepted the revision " HIVE-2833 [jira] Fix test failures caused by HIVE-2716 ". +1 Looks good to me. REVISION DETAIL https://reviews.facebook.net/D2055 BRANCH svn
        Hide
        Namit Jain added a comment -

        running tests

        Show
        Namit Jain added a comment - running tests
        Hide
        Namit Jain added a comment -

        Committed. Thanks Kevin

        Show
        Namit Jain added a comment - Committed. Thanks Kevin
        Hide
        Hudson added a comment -

        Integrated in Hive-trunk-h0.21 #1290 (See https://builds.apache.org/job/Hive-trunk-h0.21/1290/)
        HIVE-2833 Fix test failures caused by HIVE-2716
        (Kevin Wilfong via namit) (Revision 1296946)

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

        • /hive/trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
        • /hive/trunk/metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreUtils.java
        • /hive/trunk/metastore/src/java/org/apache/hadoop/hive/metastore/RetryingRawStore.java
        • /hive/trunk/metastore/src/test/org/apache/hadoop/hive/metastore/DummyJdoConnectionUrlHook.java
        • /hive/trunk/metastore/src/test/org/apache/hadoop/hive/metastore/DummyRawStoreForJdoConnection.java
        • /hive/trunk/metastore/src/test/org/apache/hadoop/hive/metastore/TestMetaStoreConnectionUrlHook.java
        Show
        Hudson added a comment - Integrated in Hive-trunk-h0.21 #1290 (See https://builds.apache.org/job/Hive-trunk-h0.21/1290/ ) HIVE-2833 Fix test failures caused by HIVE-2716 (Kevin Wilfong via namit) (Revision 1296946) Result = SUCCESS namit : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1296946 Files : /hive/trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java /hive/trunk/metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreUtils.java /hive/trunk/metastore/src/java/org/apache/hadoop/hive/metastore/RetryingRawStore.java /hive/trunk/metastore/src/test/org/apache/hadoop/hive/metastore/DummyJdoConnectionUrlHook.java /hive/trunk/metastore/src/test/org/apache/hadoop/hive/metastore/DummyRawStoreForJdoConnection.java /hive/trunk/metastore/src/test/org/apache/hadoop/hive/metastore/TestMetaStoreConnectionUrlHook.java
        Hide
        Ashutosh Chauhan added a comment -

        This issue is closed now. It was released with the fix in 0.9.0. If there is a problem, please open a new jira and link this one with that.

        Show
        Ashutosh Chauhan added a comment - This issue is closed now. It was released with the fix in 0.9.0. If there is a problem, please open a new jira and link this one with that.
        Hide
        Hudson added a comment -

        Integrated in Hive-trunk-hadoop2 #54 (See https://builds.apache.org/job/Hive-trunk-hadoop2/54/)
        HIVE-2833 Fix test failures caused by HIVE-2716
        (Kevin Wilfong via namit) (Revision 1296946)

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

        • /hive/trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
        • /hive/trunk/metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreUtils.java
        • /hive/trunk/metastore/src/java/org/apache/hadoop/hive/metastore/RetryingRawStore.java
        • /hive/trunk/metastore/src/test/org/apache/hadoop/hive/metastore/DummyJdoConnectionUrlHook.java
        • /hive/trunk/metastore/src/test/org/apache/hadoop/hive/metastore/DummyRawStoreForJdoConnection.java
        • /hive/trunk/metastore/src/test/org/apache/hadoop/hive/metastore/TestMetaStoreConnectionUrlHook.java
        Show
        Hudson added a comment - Integrated in Hive-trunk-hadoop2 #54 (See https://builds.apache.org/job/Hive-trunk-hadoop2/54/ ) HIVE-2833 Fix test failures caused by HIVE-2716 (Kevin Wilfong via namit) (Revision 1296946) Result = ABORTED namit : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1296946 Files : /hive/trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java /hive/trunk/metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreUtils.java /hive/trunk/metastore/src/java/org/apache/hadoop/hive/metastore/RetryingRawStore.java /hive/trunk/metastore/src/test/org/apache/hadoop/hive/metastore/DummyJdoConnectionUrlHook.java /hive/trunk/metastore/src/test/org/apache/hadoop/hive/metastore/DummyRawStoreForJdoConnection.java /hive/trunk/metastore/src/test/org/apache/hadoop/hive/metastore/TestMetaStoreConnectionUrlHook.java

          People

          • Assignee:
            Kevin Wilfong
            Reporter:
            Carl Steinbach
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development