Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Duplicate
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: None
    • Labels:
      None

      Description

      This is a pretty serious problem.

      If a conf variable is changed, Hive may not pick up the variable unless the metastore variables are changed.
      When any session variables are changed, it might be simpler to update the corresponding Hive conf.

        Issue Links

          Activity

          Show
          Namit Jain added a comment - https://reviews.facebook.net/differential/diff/8583/
          Hide
          Phabricator added a comment -

          kevinwilfong requested code review of "HIVE-2931 [jira] conf settings may be ignored".
          Reviewers: JIRA

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

          Unfortunately, Namit's patch did not pass the tests and he is away. I offer this patch as a different solution. The issue is that before HIVE-2716, Hive would alway have its needsRefresh set to true. Now, however, this is no longer the case. This exposed the issue that the Hive class uses config variables which are not metavariables. This seems perfectly reasonable, except that its HiveConf instance is only updated if a metavariable changes. It should be updated regardless. A metavariable changing should only determine if it needs to connect to a new metastore instance.

          This is a pretty serious problem.

          If a conf variable is changed, Hive may not pick up the variable unless the metastore variables are changed.
          When any session variables are changed, it might be simpler to update the corresponding Hive conf.

          TEST PLAN
          EMPTY

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

          AFFECTED FILES
          ql/src/java/org/apache/hadoop/hive/ql/metadata/Hive.java

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

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

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

          Show
          Phabricator added a comment - kevinwilfong requested code review of " HIVE-2931 [jira] conf settings may be ignored". Reviewers: JIRA https://issues.apache.org/jira/browse/HIVE-2931 Unfortunately, Namit's patch did not pass the tests and he is away. I offer this patch as a different solution. The issue is that before HIVE-2716 , Hive would alway have its needsRefresh set to true. Now, however, this is no longer the case. This exposed the issue that the Hive class uses config variables which are not metavariables. This seems perfectly reasonable, except that its HiveConf instance is only updated if a metavariable changes. It should be updated regardless. A metavariable changing should only determine if it needs to connect to a new metastore instance. This is a pretty serious problem. If a conf variable is changed, Hive may not pick up the variable unless the metastore variables are changed. When any session variables are changed, it might be simpler to update the corresponding Hive conf. TEST PLAN EMPTY REVISION DETAIL https://reviews.facebook.net/D2781 AFFECTED FILES ql/src/java/org/apache/hadoop/hive/ql/metadata/Hive.java MANAGE HERALD DIFFERENTIAL RULES https://reviews.facebook.net/herald/view/differential/ WHY DID I GET THIS EMAIL? https://reviews.facebook.net/herald/transcript/6309/ Tip: use the X-Herald-Rules header to filter Herald messages in your client.
          Hide
          Phabricator added a comment -

          stuart983 has commented on the revision "HIVE-2931 [jira] conf settings may be ignored".

          I'm not sure it is the right thing to do. This patch implies that, every time we call Hive.get(HiveConf), we replace the threadlocal Hive object's conf to the one passed in?

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

          Show
          Phabricator added a comment - stuart983 has commented on the revision " HIVE-2931 [jira] conf settings may be ignored". I'm not sure it is the right thing to do. This patch implies that, every time we call Hive.get(HiveConf), we replace the threadlocal Hive object's conf to the one passed in? REVISION DETAIL https://reviews.facebook.net/D2781
          Hide
          Phabricator added a comment -

          kevinwilfong has commented on the revision "HIVE-2931 [jira] conf settings may be ignored".

          Previously, this is what was effectively done, since every time
          public static Hive get(HiveConf c)
          was called needsRefresh was set to true in which case the thread local Hive instance was replaced with a new instance using the conf.

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

          Show
          Phabricator added a comment - kevinwilfong has commented on the revision " HIVE-2931 [jira] conf settings may be ignored". Previously, this is what was effectively done, since every time public static Hive get(HiveConf c) was called needsRefresh was set to true in which case the thread local Hive instance was replaced with a new instance using the conf. REVISION DETAIL https://reviews.facebook.net/D2781
          Hide
          Phabricator added a comment -

          stuart983 has commented on the revision "HIVE-2931 [jira] conf settings may be ignored".

          One thing I just don't understand is, if always doing db.conf= c works, then why we need to comparing the meta vars and only modify when there is difference? I think we must have missed something here.

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

          Show
          Phabricator added a comment - stuart983 has commented on the revision " HIVE-2931 [jira] conf settings may be ignored". One thing I just don't understand is, if always doing db.conf= c works, then why we need to comparing the meta vars and only modify when there is difference? I think we must have missed something here. REVISION DETAIL https://reviews.facebook.net/D2781
          Hide
          Phabricator added a comment -

          kevinwilfong has commented on the revision "HIVE-2931 [jira] conf settings may be ignored".

          The reason for comparing the metavars is to determine whether or not a new metastore client needs to be created or not, once the metastore client is created, the conf is not updated. So if any of the conf variables used by the metastore are modified the metastore client needs to be restarted to get those new configs. I assume this is expensive.

          If a config variable is changed which may be accessed by the Hive object, but not the metastore config variable, if that is changed, there is no need to create a new metastore client, but Hive's config should still be updated to make sure it gets the new value.

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

          Show
          Phabricator added a comment - kevinwilfong has commented on the revision " HIVE-2931 [jira] conf settings may be ignored". The reason for comparing the metavars is to determine whether or not a new metastore client needs to be created or not, once the metastore client is created, the conf is not updated. So if any of the conf variables used by the metastore are modified the metastore client needs to be restarted to get those new configs. I assume this is expensive. If a config variable is changed which may be accessed by the Hive object, but not the metastore config variable, if that is changed, there is no need to create a new metastore client, but Hive's config should still be updated to make sure it gets the new value. REVISION DETAIL https://reviews.facebook.net/D2781
          Hide
          Phabricator added a comment -

          stuart983 has accepted the revision "HIVE-2931 [jira] conf settings may be ignored".

          Communicated offline with Kevin. It looks fine for now. But I still think fixing setProcessor to update local Hive object is something we still need to do. Maybe we should create a follow-up task for that.

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

          BRANCH
          svn

          Show
          Phabricator added a comment - stuart983 has accepted the revision " HIVE-2931 [jira] conf settings may be ignored". Communicated offline with Kevin. It looks fine for now. But I still think fixing setProcessor to update local Hive object is something we still need to do. Maybe we should create a follow-up task for that. REVISION DETAIL https://reviews.facebook.net/D2781 BRANCH svn
          Hide
          Siying Dong added a comment -

          +1, will commit after tests pass.

          Show
          Siying Dong added a comment - +1, will commit after tests pass.
          Hide
          Siying Dong added a comment -

          +1, will commit after tests pass.

          Show
          Siying Dong added a comment - +1, will commit after tests pass.
          Hide
          Carl Steinbach added a comment -

          This ticket is a duplicate of HIVE-2918. I have provided a patch for HIVE-2918 which provides the same fix along with two new testcases that verify the behavior of hive.exec.max.dynamic.partitions. Please test and commit that patch instead.

          Show
          Carl Steinbach added a comment - This ticket is a duplicate of HIVE-2918 . I have provided a patch for HIVE-2918 which provides the same fix along with two new testcases that verify the behavior of hive.exec.max.dynamic.partitions. Please test and commit that patch instead.
          Hide
          Carl Steinbach added a comment -

          Also, this patch doesn't add any tests to verify that it actually fixes the problem. Please either add tests or commit HIVE-2918 instead.

          Show
          Carl Steinbach added a comment - Also, this patch doesn't add any tests to verify that it actually fixes the problem. Please either add tests or commit HIVE-2918 instead.
          Hide
          Kevin Wilfong added a comment -

          I'd prefer to use your patch because, as you said, it is nearly an identical fix, and it has test cases.

          Show
          Kevin Wilfong added a comment - I'd prefer to use your patch because, as you said, it is nearly an identical fix, and it has test cases.
          Hide
          Siying Dong added a comment -

          OK.

          Show
          Siying Dong added a comment - OK.

            People

            • Assignee:
              Kevin Wilfong
              Reporter:
              Namit Jain
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development