Details

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

      Description

      NPE in AbstractSmcConfigHook.java

        Activity

        Hide
        Carl Steinbach added a comment -

        Closing this ticket as invalid since HIVE-2986 was reverted in HIVE-3002.

        Show
        Carl Steinbach added a comment - Closing this ticket as invalid since HIVE-2986 was reverted in HIVE-3002 .
        Hide
        Kevin Wilfong added a comment -

        @Carl
        Can I commit this patch (our internal environment is severely broken without this fix)?

        We can move the discussion to HIVE-2985

        Show
        Kevin Wilfong added a comment - @Carl Can I commit this patch (our internal environment is severely broken without this fix)? We can move the discussion to HIVE-2985
        Hide
        Namit Jain added a comment -

        @Carl, this will be a iterative process, and the changes are only going in contrib.
        The internal, non-public APIs are locked for facebook anyway, and we run into these
        problems at deployment time. It would be very difficult to keep it in sync. in a separate
        branch.

        Right now, there are no tests for these, so any change by any contributor cannot break anything
        in these hooks. As I will be adding tests for these hooks, I will be converting these hooks to
        not use any private data-structures/APIs. That is the reason, I did not blindly any existing tests.
        In the last few pushes, we have run into random issues, which could have been addressed before had
        this test been in place.

        As I said before, the purpose of HIVE-2670 is similar, but having the facebook deployment in the open
        makes us feel much more comfortable in running the recent branch of hive. I really dont want Facebook
        to have its own branch for hive, which makes it very difficult to sync. up later. The problem is that
        if I keep developing in a separate branch, i will diverge very quickly and be never able to converge.

        Show
        Namit Jain added a comment - @Carl, this will be a iterative process, and the changes are only going in contrib. The internal, non-public APIs are locked for facebook anyway, and we run into these problems at deployment time. It would be very difficult to keep it in sync. in a separate branch. Right now, there are no tests for these, so any change by any contributor cannot break anything in these hooks. As I will be adding tests for these hooks, I will be converting these hooks to not use any private data-structures/APIs. That is the reason, I did not blindly any existing tests. In the last few pushes, we have run into random issues, which could have been addressed before had this test been in place. As I said before, the purpose of HIVE-2670 is similar, but having the facebook deployment in the open makes us feel much more comfortable in running the recent branch of hive. I really dont want Facebook to have its own branch for hive, which makes it very difficult to sync. up later. The problem is that if I keep developing in a separate branch, i will diverge very quickly and be never able to converge.
        Hide
        Carl Steinbach added a comment -

        @Namit: I'm not concerned that these hooks are going to break
        anything. Actually, I have the opposite concern, namely that the
        new hooks and tests are going to make it much harder for Hive
        developers to improve internal, non-public APIs by locking down
        internal, non-public implementation details. We already have more
        than a thousand tests designed to catch changes to the public
        APIs (HQL, the public listener and hook interfaces, the Thrift
        APIs, etc), and HIVE-2670 is set to add better support for
        running these tests in the sort of e2e scenarios that members of
        the community encounter in production environments.

        Since this is such a big change I propose that this work be done
        on a separate branch, either here or on github, and then when
        it's finished we can give the committers a chance to evaluate it
        as a whole. How does that sound?

        Show
        Carl Steinbach added a comment - @Namit: I'm not concerned that these hooks are going to break anything. Actually, I have the opposite concern, namely that the new hooks and tests are going to make it much harder for Hive developers to improve internal, non-public APIs by locking down internal, non-public implementation details. We already have more than a thousand tests designed to catch changes to the public APIs (HQL, the public listener and hook interfaces, the Thrift APIs, etc), and HIVE-2670 is set to add better support for running these tests in the sort of e2e scenarios that members of the community encounter in production environments. Since this is such a big change I propose that this work be done on a separate branch, either here or on github, and then when it's finished we can give the committers a chance to evaluate it as a whole. How does that sound?
        Hide
        Namit Jain added a comment -

        @Carl, this is a pretty big change, and I did not want to wait for all the cleanups to happen before I could get it in.
        Having said that, I will be providing 2 sets of tests for these soon:
        1. Unit tests
        2. End to End tests which will use these hooks.

        Once these tests are in, it would be very easy to modify the hooks. It is just I did not want to wait for the whole patch to go in a single patch -
        this is not touching any hive code, and should not break anything.

        Show
        Namit Jain added a comment - @Carl, this is a pretty big change, and I did not want to wait for all the cleanups to happen before I could get it in. Having said that, I will be providing 2 sets of tests for these soon: 1. Unit tests 2. End to End tests which will use these hooks. Once these tests are in, it would be very easy to modify the hooks. It is just I did not want to wait for the whole patch to go in a single patch - this is not touching any hive code, and should not break anything.
        Hide
        Carl Steinbach added a comment -

        @Namit: My main concern is that many of the hooks which were committed in HIVE-2986 reference non-public APIs. For example, I see references to SessionState, QueryPlan, DDLTask, ExplainTask, HiveOperation, etc. It looks like one consequence of adding these hooks is that developers will now have to treat them as de facto public APIs. Does that sound accurate? If so, what's the proposed policy for handling future changes to these internal APIs? What will be the process for modifying the hooks which were added in HIVE-2986, and where will the tests that reference these hooks live? If a change breaks one of these tests, who is responsible for fixing it?

        Show
        Carl Steinbach added a comment - @Namit: My main concern is that many of the hooks which were committed in HIVE-2986 reference non-public APIs. For example, I see references to SessionState, QueryPlan, DDLTask, ExplainTask, HiveOperation, etc. It looks like one consequence of adding these hooks is that developers will now have to treat them as de facto public APIs. Does that sound accurate? If so, what's the proposed policy for handling future changes to these internal APIs? What will be the process for modifying the hooks which were added in HIVE-2986 , and where will the tests that reference these hooks live? If a change breaks one of these tests, who is responsible for fixing it?
        Hide
        Namit Jain added a comment -

        @Carl, facebook is pretty close to deploying the trunk, which has its own advantages and disadvantages.
        The deployment framework at facebook is slightly different (maybe) from others - so, the first step is to
        have the ability to create the environment in open source. This includes having all the hooks that we have used,
        and the hive-site.xml.

        The high level goals of HIVE-2670 and this are similar, but the major difference is the hooks, hive-site.xml etc.
        Once this is done, I also plan to have a script which can be run (via jenkins) automatically, assuming the users
        have a mysql db. and HADOOP_HOME installed. This way, for a new commit, Facebook will be pretty confident that it
        is not breaking anything. Also, it allows outside Facebook contributors to debug the problems we encounter. In absence
        of this environment, we (inside Faceook) end up debugging all these issues.

        For the first cut, it need not even be integrated with ant - but, that would be good to do eventually.
        Also, I am not planning performance regressions initially, but again that would be good to have.

        Show
        Namit Jain added a comment - @Carl, facebook is pretty close to deploying the trunk, which has its own advantages and disadvantages. The deployment framework at facebook is slightly different (maybe) from others - so, the first step is to have the ability to create the environment in open source. This includes having all the hooks that we have used, and the hive-site.xml. The high level goals of HIVE-2670 and this are similar, but the major difference is the hooks, hive-site.xml etc. Once this is done, I also plan to have a script which can be run (via jenkins) automatically, assuming the users have a mysql db. and HADOOP_HOME installed. This way, for a new commit, Facebook will be pretty confident that it is not breaking anything. Also, it allows outside Facebook contributors to debug the problems we encounter. In absence of this environment, we (inside Faceook) end up debugging all these issues. For the first cut, it need not even be integrated with ant - but, that would be good to do eventually. Also, I am not planning performance regressions initially, but again that would be good to have.
        Hide
        Kevin Wilfong added a comment -

        @Namit: The tests pass, but see Carl's comment.

        @Carl: This patch, and the one it fixes, HIVE-2986, are just the first step in an effort by us to open source our environment, specifically the hooks we use, to initially allow us to be better able to work with other open source contributors to debug issues when a patch breaks them (as has happened in the past). Neither patch provides (nor attempts to provide) any sort of test framework.

        Creating a test framework is a goal, and some parts may overlap in the patch you mention in HIVE-2985 which should be investigated. But again, the point of these two patches is not to create a test framework, and do not overlap with the patch you mention in HIVE-2985.

        Show
        Kevin Wilfong added a comment - @Namit: The tests pass, but see Carl's comment. @Carl: This patch, and the one it fixes, HIVE-2986 , are just the first step in an effort by us to open source our environment, specifically the hooks we use, to initially allow us to be better able to work with other open source contributors to debug issues when a patch breaks them (as has happened in the past). Neither patch provides (nor attempts to provide) any sort of test framework. Creating a test framework is a goal, and some parts may overlap in the patch you mention in HIVE-2985 which should be investigated. But again, the point of these two patches is not to create a test framework, and do not overlap with the patch you mention in HIVE-2985 .
        Hide
        Carl Steinbach added a comment -

        @Kevin: Can either you or Namit please respond to my question in HIVE-2985 before committing this patch? Thanks.

        Show
        Carl Steinbach added a comment - @Kevin: Can either you or Namit please respond to my question in HIVE-2985 before committing this patch? Thanks.
        Hide
        Phabricator added a comment -

        kevinwilfong has accepted the revision "HIVE-2996 [jira] minor bug in hooks (NPE)".

        +1 Running tests

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

        BRANCH
        svn

        Show
        Phabricator added a comment - kevinwilfong has accepted the revision " HIVE-2996 [jira] minor bug in hooks (NPE)". +1 Running tests REVISION DETAIL https://reviews.facebook.net/D2991 BRANCH svn
        Hide
        Phabricator added a comment -

        njain requested code review of "HIVE-2996 [jira] minor bug in hooks (NPE)".
        Reviewers: JIRA

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

        HIVE-2996 minor bug in hooks (NPE)

        NPE in AbstractSmcConfigHook.java

        TEST PLAN
        EMPTY

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

        AFFECTED FILES
        contrib/src/java/org/apache/hadoop/hive/ql/hooks/AbstractSmcConfigHook.java

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

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

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

        Show
        Phabricator added a comment - njain requested code review of " HIVE-2996 [jira] minor bug in hooks (NPE)". Reviewers: JIRA https://issues.apache.org/jira/browse/HIVE-2996 HIVE-2996 minor bug in hooks (NPE) NPE in AbstractSmcConfigHook.java TEST PLAN EMPTY REVISION DETAIL https://reviews.facebook.net/D2991 AFFECTED FILES contrib/src/java/org/apache/hadoop/hive/ql/hooks/AbstractSmcConfigHook.java MANAGE HERALD DIFFERENTIAL RULES https://reviews.facebook.net/herald/view/differential/ WHY DID I GET THIS EMAIL? https://reviews.facebook.net/herald/transcript/6813/ Tip: use the X-Herald-Rules header to filter Herald messages in your client.

          People

          • Assignee:
            Unassigned
            Reporter:
            Namit Jain
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development