Details

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

      Description

      Ideally, there should be one common entity instead of readentity/writeentity.

      Unfortunately, that would be a backward incompatible change since users os hive might have written
      there own hooks, where they are using readentity/writeentity.
      We should atleast create a common class, and then we can deprecate read/write entity later, for a new release.

      For now, I propose to make a backward compatible change.

        Activity

        Hide
        Hudson added a comment -

        Integrated in Hive-trunk-hadoop2 #54 (See https://builds.apache.org/job/Hive-trunk-hadoop2/54/)
        HIVE-2838. cleanup readentity/writeentity. (namit via kevinwilfong) (Revision 1298699)

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

        • /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/hooks/Entity.java
        • /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/hooks/ReadEntity.java
        • /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/hooks/WriteEntity.java
        Show
        Hudson added a comment - Integrated in Hive-trunk-hadoop2 #54 (See https://builds.apache.org/job/Hive-trunk-hadoop2/54/ ) HIVE-2838 . cleanup readentity/writeentity. (namit via kevinwilfong) (Revision 1298699) Result = ABORTED kevinwilfong : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1298699 Files : /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/hooks/Entity.java /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/hooks/ReadEntity.java /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/hooks/WriteEntity.java
        Hide
        Phabricator added a comment -

        njain has abandoned the revision "HIVE-2838 [jira] cleanup readentity/writeentity".

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

        Show
        Phabricator added a comment - njain has abandoned the revision " HIVE-2838 [jira] cleanup readentity/writeentity". REVISION DETAIL https://reviews.facebook.net/D2193
        Ashutosh Chauhan made changes -
        Status Resolved [ 5 ] Closed [ 6 ]
        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-h0.21 #1300 (See https://builds.apache.org/job/Hive-trunk-h0.21/1300/)
        HIVE-2838. cleanup readentity/writeentity. (namit via kevinwilfong) (Revision 1298699)

        Result = FAILURE
        kevinwilfong : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1298699
        Files :

        • /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/hooks/Entity.java
        • /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/hooks/ReadEntity.java
        • /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/hooks/WriteEntity.java
        Show
        Hudson added a comment - Integrated in Hive-trunk-h0.21 #1300 (See https://builds.apache.org/job/Hive-trunk-h0.21/1300/ ) HIVE-2838 . cleanup readentity/writeentity. (namit via kevinwilfong) (Revision 1298699) Result = FAILURE kevinwilfong : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1298699 Files : /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/hooks/Entity.java /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/hooks/ReadEntity.java /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/hooks/WriteEntity.java
        Kevin Wilfong made changes -
        Status Patch Available [ 10002 ] Resolved [ 5 ]
        Fix Version/s 0.9.0 [ 12317742 ]
        Resolution Fixed [ 1 ]
        Hide
        Kevin Wilfong added a comment -

        Committed, thanks Namit.

        Show
        Kevin Wilfong added a comment - Committed, thanks Namit.
        Hide
        Phabricator added a comment -

        kevinwilfong has accepted the revision "HIVE-2838 [jira] cleanup readentity/writeentity".

        +1

        Will commit after tests pass.

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

        BRANCH
        svn

        Show
        Phabricator added a comment - kevinwilfong has accepted the revision " HIVE-2838 [jira] cleanup readentity/writeentity". +1 Will commit after tests pass. REVISION DETAIL https://reviews.facebook.net/D2193 BRANCH svn
        Phabricator made changes -
        Attachment HIVE-2838.D2193.2.patch [ 12517611 ]
        Hide
        Phabricator added a comment -

        njain updated the revision "HIVE-2838 [jira] cleanup readentity/writeentity".
        Reviewers: JIRA, kevinwilfong

        Comments

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

        AFFECTED FILES
        ql/src/java/org/apache/hadoop/hive/ql/hooks/WriteEntity.java
        ql/src/java/org/apache/hadoop/hive/ql/hooks/ReadEntity.java
        ql/src/java/org/apache/hadoop/hive/ql/hooks/Entity.java

        Show
        Phabricator added a comment - njain updated the revision " HIVE-2838 [jira] cleanup readentity/writeentity". Reviewers: JIRA, kevinwilfong Comments REVISION DETAIL https://reviews.facebook.net/D2193 AFFECTED FILES ql/src/java/org/apache/hadoop/hive/ql/hooks/WriteEntity.java ql/src/java/org/apache/hadoop/hive/ql/hooks/ReadEntity.java ql/src/java/org/apache/hadoop/hive/ql/hooks/Entity.java
        Namit Jain made changes -
        Status Open [ 1 ] Patch Available [ 10002 ]
        Hide
        Phabricator added a comment -

        kevinwilfong has requested changes to the revision "HIVE-2838 [jira] cleanup readentity/writeentity".

        The code looks good.

        Noticed a few relics left over from WriteEntity in the comments in Entity.

        INLINE COMMENTS
        ql/src/java/org/apache/hadoop/hive/ql/hooks/Entity.java:30-31 Could you fix this comment.
        ql/src/java/org/apache/hadoop/hive/ql/hooks/Entity.java:37 This one too.
        ql/src/java/org/apache/hadoop/hive/ql/hooks/Entity.java:70 Could you split this into two.
        ql/src/java/org/apache/hadoop/hive/ql/hooks/Entity.java:133 Could you change written to to accessed or something like that.
        ql/src/java/org/apache/hadoop/hive/ql/hooks/Entity.java:152 here too.
        ql/src/java/org/apache/hadoop/hive/ql/hooks/Entity.java:180 here too.

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

        BRANCH
        svn

        Show
        Phabricator added a comment - kevinwilfong has requested changes to the revision " HIVE-2838 [jira] cleanup readentity/writeentity". The code looks good. Noticed a few relics left over from WriteEntity in the comments in Entity. INLINE COMMENTS ql/src/java/org/apache/hadoop/hive/ql/hooks/Entity.java:30-31 Could you fix this comment. ql/src/java/org/apache/hadoop/hive/ql/hooks/Entity.java:37 This one too. ql/src/java/org/apache/hadoop/hive/ql/hooks/Entity.java:70 Could you split this into two. ql/src/java/org/apache/hadoop/hive/ql/hooks/Entity.java:133 Could you change written to to accessed or something like that. ql/src/java/org/apache/hadoop/hive/ql/hooks/Entity.java:152 here too. ql/src/java/org/apache/hadoop/hive/ql/hooks/Entity.java:180 here too. REVISION DETAIL https://reviews.facebook.net/D2193 BRANCH svn
        Phabricator made changes -
        Field Original Value New Value
        Attachment HIVE-2838.D2193.1.patch [ 12517589 ]
        Hide
        Phabricator added a comment -

        njain requested code review of "HIVE-2838 [jira] cleanup readentity/writeentity".
        Reviewers: JIRA

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

        HIVE-2838 Cleaup read/write Entities

        Ideally, there should be one common entity instead of readentity/writeentity.

        Unfortunately, that would be a backward incompatible change since users os hive might have written
        there own hooks, where they are using readentity/writeentity.
        We should atleast create a common class, and then we can deprecate read/write entity later, for a new release.

        For now, I propose to make a backward compatible change.

        TEST PLAN
        EMPTY

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

        AFFECTED FILES
        ql/src/java/org/apache/hadoop/hive/ql/hooks/WriteEntity.java
        ql/src/java/org/apache/hadoop/hive/ql/hooks/ReadEntity.java
        ql/src/java/org/apache/hadoop/hive/ql/hooks/Entity.java

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

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

        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-2838 [jira] cleanup readentity/writeentity". Reviewers: JIRA https://issues.apache.org/jira/browse/HIVE-2838 HIVE-2838 Cleaup read/write Entities Ideally, there should be one common entity instead of readentity/writeentity. Unfortunately, that would be a backward incompatible change since users os hive might have written there own hooks, where they are using readentity/writeentity. We should atleast create a common class, and then we can deprecate read/write entity later, for a new release. For now, I propose to make a backward compatible change. TEST PLAN EMPTY REVISION DETAIL https://reviews.facebook.net/D2193 AFFECTED FILES ql/src/java/org/apache/hadoop/hive/ql/hooks/WriteEntity.java ql/src/java/org/apache/hadoop/hive/ql/hooks/ReadEntity.java ql/src/java/org/apache/hadoop/hive/ql/hooks/Entity.java MANAGE HERALD DIFFERENTIAL RULES https://reviews.facebook.net/herald/view/differential/ WHY DID I GET THIS EMAIL? https://reviews.facebook.net/herald/transcript/4815/ Tip: use the X-Herald-Rules header to filter Herald messages in your client.
        Hide
        Namit Jain added a comment -

        There is a lot of common stuff between ReadEntity and WriteEntity - table, partition, type etc.
        If you look at the code, it is ~80% same.

        Chances are the custom hooks that you are writing for processing readEntity and writeEntity are similar, and
        are duplicating a lot of logic.

        I was proposing the following:

        Common Class (Entity)

        Both ReadEntity and WriteEntity extend Entity.

        Nothing needs to be changed for existing hooks.

        Show
        Namit Jain added a comment - There is a lot of common stuff between ReadEntity and WriteEntity - table, partition, type etc. If you look at the code, it is ~80% same. Chances are the custom hooks that you are writing for processing readEntity and writeEntity are similar, and are duplicating a lot of logic. I was proposing the following: Common Class (Entity) Both ReadEntity and WriteEntity extend Entity. Nothing needs to be changed for existing hooks.
        Hide
        Ashutosh Chauhan added a comment -

        @Namit,
        Can you briefly explain why there should be one common entity instead of read/write entity?

        Show
        Ashutosh Chauhan added a comment - @Namit, Can you briefly explain why there should be one common entity instead of read/write entity?
        Namit Jain created issue -

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development