Uploaded image for project: 'Hive'
  1. Hive
  2. HIVE-16172

Switch to a fairness lock to synchronize HS2 thrift client

    Details

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

      Description

      A synchronized block is used in "org.apache.hive.jdbc.HiveConnection.SynchronizedHandler.invoke(Object, Method, Object[])" to synchronize the client invocations. The problem is that it does not guarantee any fairness. One issue we were seeing is that a cancellation request was not able to be issued to HS2 until all the getOperationStatus() calls are finished from a while loop. Thus the cancellation cannot take effect.

        Activity

        Hide
        taoli-hwx Tao Li added a comment -

        I have verified the change by cancelling the SQL query while it's executed. Without the change, the query was not able to be cancelled (see details in the JIRA description). With the change the cancellation worked. Note that we need to set fairness to true when creating the lock, otherwise the effect is similar to the original synchronized block.

        Daniel Dai Can you please review this change?

        Show
        taoli-hwx Tao Li added a comment - I have verified the change by cancelling the SQL query while it's executed. Without the change, the query was not able to be cancelled (see details in the JIRA description). With the change the cancellation worked. Note that we need to set fairness to true when creating the lock, otherwise the effect is similar to the original synchronized block. Daniel Dai Can you please review this change?
        Hide
        taoli-hwx Tao Li added a comment -

        cc Gopal V in case there is any perf concern. Forcing the fairness does hurt the throughput, compared with no fairness, according to: https://docs.oracle.com/javase/7/docs/api/java/util/concurrent/locks/ReentrantLock.html

        Show
        taoli-hwx Tao Li added a comment - cc Gopal V in case there is any perf concern. Forcing the fairness does hurt the throughput, compared with no fairness, according to: https://docs.oracle.com/javase/7/docs/api/java/util/concurrent/locks/ReentrantLock.html
        Hide
        hiveqa Hive QA added a comment -

        Here are the results of testing the latest attachment:
        https://issues.apache.org/jira/secure/attachment/12857262/HIVE-16172.1.patch

        ERROR: -1 due to no test(s) being added or modified.

        SUCCESS: +1 due to 10336 tests passed

        Test results: https://builds.apache.org/job/PreCommit-HIVE-Build/4062/testReport
        Console output: https://builds.apache.org/job/PreCommit-HIVE-Build/4062/console
        Test logs: http://104.198.109.242/logs/PreCommit-HIVE-Build-4062/

        Messages:

        Executing org.apache.hive.ptest.execution.TestCheckPhase
        Executing org.apache.hive.ptest.execution.PrepPhase
        Executing org.apache.hive.ptest.execution.ExecutionPhase
        Executing org.apache.hive.ptest.execution.ReportingPhase
        

        This message is automatically generated.

        ATTACHMENT ID: 12857262 - PreCommit-HIVE-Build

        Show
        hiveqa Hive QA added a comment - Here are the results of testing the latest attachment: https://issues.apache.org/jira/secure/attachment/12857262/HIVE-16172.1.patch ERROR: -1 due to no test(s) being added or modified. SUCCESS: +1 due to 10336 tests passed Test results: https://builds.apache.org/job/PreCommit-HIVE-Build/4062/testReport Console output: https://builds.apache.org/job/PreCommit-HIVE-Build/4062/console Test logs: http://104.198.109.242/logs/PreCommit-HIVE-Build-4062/ Messages: Executing org.apache.hive.ptest.execution.TestCheckPhase Executing org.apache.hive.ptest.execution.PrepPhase Executing org.apache.hive.ptest.execution.ExecutionPhase Executing org.apache.hive.ptest.execution.ReportingPhase This message is automatically generated. ATTACHMENT ID: 12857262 - PreCommit-HIVE-Build
        Hide
        vgumashta Vaibhav Gumashta added a comment -

        +1 on the change. Gopal V you might want to take a look as well.

        Show
        vgumashta Vaibhav Gumashta added a comment - +1 on the change. Gopal V you might want to take a look as well.
        Hide
        gopalv Gopal V added a comment -

        I don't think there's enough contention on this lock from many threads - last I counted it was just 2 threads, so the performance should remain relatively identical.

        Without the change, the query was not able to be cancelled (see details in the JIRA description).

        Even if this is a slight performance hit, correctness over performance.

        Show
        gopalv Gopal V added a comment - I don't think there's enough contention on this lock from many threads - last I counted it was just 2 threads, so the performance should remain relatively identical. Without the change, the query was not able to be cancelled (see details in the JIRA description). Even if this is a slight performance hit, correctness over performance.
        Hide
        vgumashta Vaibhav Gumashta added a comment -

        Gopal V Thanks for the feedback. I'll commit this shortly.

        Show
        vgumashta Vaibhav Gumashta added a comment - Gopal V Thanks for the feedback. I'll commit this shortly.
        Hide
        vgumashta Vaibhav Gumashta added a comment -

        Committed. Thanks Tao Li

        Show
        vgumashta Vaibhav Gumashta added a comment - Committed. Thanks Tao Li

          People

          • Assignee:
            taoli-hwx Tao Li
            Reporter:
            taoli-hwx Tao Li
          • Votes:
            0 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development