Hive
  1. Hive
  2. HIVE-2123

CommandNeedRetryException needs release locks

    Details

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

      Description

      now when CommandNeedRetryException is thrown, locks are not released. Not sure whether it will cause problem, since the same locks will be acquired when retrying it. It is anyway something we need to fix. Also we can do some little code cleaning up to make future mistakes less likely.

      1. HIVE-2123.1.patch
        6 kB
        Siying Dong
      2. HIVE-2123.2.patch
        8 kB
        Siying Dong
      3. HIVE-2123.3.patch
        8 kB
        Siying Dong
      4. HIVE-2123.4.patch
        9 kB
        Siying Dong

        Activity

        Hide
        He Yongqiang added a comment -

        i will take a look.

        Show
        He Yongqiang added a comment - i will take a look.
        Hide
        He Yongqiang added a comment -

        can you create a review board for this one?

        Show
        He Yongqiang added a comment - can you create a review board for this one?
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/646/
        -----------------------------------------------------------

        Review request for hive, Yongqiang He and namit jain.

        Summary
        -------

        now when CommandNeedRetryException is thrown, locks are not released. Not sure whether it will cause problem, since the same locks will be acquired when retrying it. It is anyway something we need to fix. Also we can do some little code cleaning up to make future mistakes less likely.

        This addresses bug HIVE-2123.
        https://issues.apache.org/jira/browse/HIVE-2123

        Diffs


        trunk/cli/src/java/org/apache/hadoop/hive/cli/CliDriver.java 1095838
        trunk/ql/src/java/org/apache/hadoop/hive/ql/Driver.java 1095838

        Diff: https://reviews.apache.org/r/646/diff

        Testing
        -------

        Thanks,

        Siying

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/646/ ----------------------------------------------------------- Review request for hive, Yongqiang He and namit jain. Summary ------- now when CommandNeedRetryException is thrown, locks are not released. Not sure whether it will cause problem, since the same locks will be acquired when retrying it. It is anyway something we need to fix. Also we can do some little code cleaning up to make future mistakes less likely. This addresses bug HIVE-2123 . https://issues.apache.org/jira/browse/HIVE-2123 Diffs trunk/cli/src/java/org/apache/hadoop/hive/cli/CliDriver.java 1095838 trunk/ql/src/java/org/apache/hadoop/hive/ql/Driver.java 1095838 Diff: https://reviews.apache.org/r/646/diff Testing ------- Thanks, Siying
        Hide
        He Yongqiang added a comment -

        +1, running tests

        Show
        He Yongqiang added a comment - +1, running tests
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/646/#review527
        -----------------------------------------------------------

        trunk/cli/src/java/org/apache/hadoop/hive/cli/CliDriver.java
        <https://reviews.apache.org/r/646/#comment1091>

        There's a try/catch block here, so why not do this the Java way and throw an exception instead of returning a status code?

        trunk/ql/src/java/org/apache/hadoop/hive/ql/Driver.java
        <https://reviews.apache.org/r/646/#comment1092>

        Can you replace this with a DriverException class?

        trunk/ql/src/java/org/apache/hadoop/hive/ql/Driver.java
        <https://reviews.apache.org/r/646/#comment1093>

        spacing

        • Carl

        On 2011-04-22 00:11:43, Siying Dong wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/646/

        -----------------------------------------------------------

        (Updated 2011-04-22 00:11:43)

        Review request for hive, Yongqiang He and namit jain.

        Summary

        -------

        now when CommandNeedRetryException is thrown, locks are not released. Not sure whether it will cause problem, since the same locks will be acquired when retrying it. It is anyway something we need to fix. Also we can do some little code cleaning up to make future mistakes less likely.

        This addresses bug HIVE-2123.

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

        Diffs

        -----

        trunk/cli/src/java/org/apache/hadoop/hive/cli/CliDriver.java 1095838

        trunk/ql/src/java/org/apache/hadoop/hive/ql/Driver.java 1095838

        Diff: https://reviews.apache.org/r/646/diff

        Testing

        -------

        Thanks,

        Siying

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/646/#review527 ----------------------------------------------------------- trunk/cli/src/java/org/apache/hadoop/hive/cli/CliDriver.java < https://reviews.apache.org/r/646/#comment1091 > There's a try/catch block here, so why not do this the Java way and throw an exception instead of returning a status code? trunk/ql/src/java/org/apache/hadoop/hive/ql/Driver.java < https://reviews.apache.org/r/646/#comment1092 > Can you replace this with a DriverException class? trunk/ql/src/java/org/apache/hadoop/hive/ql/Driver.java < https://reviews.apache.org/r/646/#comment1093 > spacing Carl On 2011-04-22 00:11:43, Siying Dong wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/646/ ----------------------------------------------------------- (Updated 2011-04-22 00:11:43) Review request for hive, Yongqiang He and namit jain. Summary ------- now when CommandNeedRetryException is thrown, locks are not released. Not sure whether it will cause problem, since the same locks will be acquired when retrying it. It is anyway something we need to fix. Also we can do some little code cleaning up to make future mistakes less likely. This addresses bug HIVE-2123 . https://issues.apache.org/jira/browse/HIVE-2123 Diffs ----- trunk/cli/src/java/org/apache/hadoop/hive/cli/CliDriver.java 1095838 trunk/ql/src/java/org/apache/hadoop/hive/ql/Driver.java 1095838 Diff: https://reviews.apache.org/r/646/diff Testing ------- Thanks, Siying
        Hide
        Carl Steinbach added a comment -

        @Yongqiang: Please give Siying a chance to respond to my comments before committing.

        Show
        Carl Steinbach added a comment - @Yongqiang: Please give Siying a chance to respond to my comments before committing.
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/646/#review529
        -----------------------------------------------------------

        trunk/cli/src/java/org/apache/hadoop/hive/cli/CliDriver.java
        <https://reviews.apache.org/r/646/#comment1097>

        I tried to make some minimum change, but it looks much more difficult than I thought. The whole structure is built based on return codes, instead of exceptions. What I can do is that I can make Driver.run() not to throw any exception. In that case, we don't get two approaches twisted.

        • Siying

        On 2011-04-22 00:11:43, Siying Dong wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/646/

        -----------------------------------------------------------

        (Updated 2011-04-22 00:11:43)

        Review request for hive, Yongqiang He and namit jain.

        Summary

        -------

        now when CommandNeedRetryException is thrown, locks are not released. Not sure whether it will cause problem, since the same locks will be acquired when retrying it. It is anyway something we need to fix. Also we can do some little code cleaning up to make future mistakes less likely.

        This addresses bug HIVE-2123.

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

        Diffs

        -----

        trunk/cli/src/java/org/apache/hadoop/hive/cli/CliDriver.java 1095838

        trunk/ql/src/java/org/apache/hadoop/hive/ql/Driver.java 1095838

        Diff: https://reviews.apache.org/r/646/diff

        Testing

        -------

        Thanks,

        Siying

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/646/#review529 ----------------------------------------------------------- trunk/cli/src/java/org/apache/hadoop/hive/cli/CliDriver.java < https://reviews.apache.org/r/646/#comment1097 > I tried to make some minimum change, but it looks much more difficult than I thought. The whole structure is built based on return codes, instead of exceptions. What I can do is that I can make Driver.run() not to throw any exception. In that case, we don't get two approaches twisted. Siying On 2011-04-22 00:11:43, Siying Dong wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/646/ ----------------------------------------------------------- (Updated 2011-04-22 00:11:43) Review request for hive, Yongqiang He and namit jain. Summary ------- now when CommandNeedRetryException is thrown, locks are not released. Not sure whether it will cause problem, since the same locks will be acquired when retrying it. It is anyway something we need to fix. Also we can do some little code cleaning up to make future mistakes less likely. This addresses bug HIVE-2123 . https://issues.apache.org/jira/browse/HIVE-2123 Diffs ----- trunk/cli/src/java/org/apache/hadoop/hive/cli/CliDriver.java 1095838 trunk/ql/src/java/org/apache/hadoop/hive/ql/Driver.java 1095838 Diff: https://reviews.apache.org/r/646/diff Testing ------- Thanks, Siying
        Hide
        Siying Dong added a comment -

        I changed Driver.execute() not to throw any exception. CommandNeedRetryException is translated to another return status code in this level.

        Show
        Siying Dong added a comment - I changed Driver.execute() not to throw any exception. CommandNeedRetryException is translated to another return status code in this level.
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/646/
        -----------------------------------------------------------

        (Updated 2011-04-22 04:22:16.136899)

        Review request for hive, Yongqiang He and namit jain.

        Changes
        -------

        execute() not to throw any exception.

        Summary
        -------

        now when CommandNeedRetryException is thrown, locks are not released. Not sure whether it will cause problem, since the same locks will be acquired when retrying it. It is anyway something we need to fix. Also we can do some little code cleaning up to make future mistakes less likely.

        This addresses bug HIVE-2123.
        https://issues.apache.org/jira/browse/HIVE-2123

        Diffs (updated)


        trunk/cli/src/java/org/apache/hadoop/hive/cli/CliDriver.java 1095838
        trunk/hwi/src/java/org/apache/hadoop/hive/hwi/HWISessionItem.java 1095838
        trunk/ql/src/java/org/apache/hadoop/hive/ql/Driver.java 1095838
        trunk/ql/src/java/org/apache/hadoop/hive/ql/processors/CommandProcessor.java 1095838

        Diff: https://reviews.apache.org/r/646/diff

        Testing
        -------

        Thanks,

        Siying

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/646/ ----------------------------------------------------------- (Updated 2011-04-22 04:22:16.136899) Review request for hive, Yongqiang He and namit jain. Changes ------- execute() not to throw any exception. Summary ------- now when CommandNeedRetryException is thrown, locks are not released. Not sure whether it will cause problem, since the same locks will be acquired when retrying it. It is anyway something we need to fix. Also we can do some little code cleaning up to make future mistakes less likely. This addresses bug HIVE-2123 . https://issues.apache.org/jira/browse/HIVE-2123 Diffs (updated) trunk/cli/src/java/org/apache/hadoop/hive/cli/CliDriver.java 1095838 trunk/hwi/src/java/org/apache/hadoop/hive/hwi/HWISessionItem.java 1095838 trunk/ql/src/java/org/apache/hadoop/hive/ql/Driver.java 1095838 trunk/ql/src/java/org/apache/hadoop/hive/ql/processors/CommandProcessor.java 1095838 Diff: https://reviews.apache.org/r/646/diff Testing ------- Thanks, Siying
        Hide
        Siying Dong added a comment -

        @Carl?

        Show
        Siying Dong added a comment - @Carl?
        Hide
        Carl Steinbach added a comment -

        @Siying: Thanks for making the change.

        +1 on this patch, but I'm -1 on the overall approach of using status return codes instead of exceptions. Hopefully we can replace the status codes with exceptions during some future cleanup effort.

        Show
        Carl Steinbach added a comment - @Siying: Thanks for making the change. +1 on this patch, but I'm -1 on the overall approach of using status return codes instead of exceptions. Hopefully we can replace the status codes with exceptions during some future cleanup effort.
        Hide
        He Yongqiang added a comment -

        looks good to me. will commit after tests pass.

        Show
        He Yongqiang added a comment - looks good to me. will commit after tests pass.
        Hide
        He Yongqiang added a comment -

        Siying, can you rebase the patch. I got some small conflict when trying to commit.

        Show
        He Yongqiang added a comment - Siying, can you rebase the patch. I got some small conflict when trying to commit.
        Hide
        Siying Dong added a comment -

        rebase

        Show
        Siying Dong added a comment - rebase
        Hide
        He Yongqiang added a comment -

        Siying, HIVE-2123.3.patch can not pass compilation. And HIVE-2123.4.patch does not have apache license granted.

        Show
        He Yongqiang added a comment - Siying, HIVE-2123 .3.patch can not pass compilation. And HIVE-2123 .4.patch does not have apache license granted.
        Hide
        Siying Dong added a comment -

        License granted.

        Show
        Siying Dong added a comment - License granted.
        Hide
        John Sichi added a comment -

        This one has been sitting in Patch Available queue for a while...anything holding it up?

        Show
        John Sichi added a comment - This one has been sitting in Patch Available queue for a while...anything holding it up?
        Hide
        Carl Steinbach added a comment -

        @Siying: Can you please rebase the patch?

        Show
        Carl Steinbach added a comment - @Siying: Can you please rebase the patch?

          People

          • Assignee:
            Siying Dong
            Reporter:
            Siying Dong
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:

              Development