You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hive.apache.org by Vaibhav Gumashta <vg...@hortonworks.com> on 2014/03/19 12:02:31 UTC
Review Request 19395: JDBC cancel will not work with current HiveServer2
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19395/
-----------------------------------------------------------
Review request for hive, Prasad Mujumdar and Thejas Nair.
Bugs: HIVE-6472
https://issues.apache.org/jira/browse/HIVE-6472
Repository: hive-git
Description
-------
https://issues.apache.org/jira/browse/HIVE-6472
Diffs
-----
jdbc/src/java/org/apache/hive/jdbc/HiveStatement.java 01e6ea7
service/src/java/org/apache/hive/service/cli/OperationState.java a023908
service/src/java/org/apache/hive/service/cli/operation/ExecuteStatementOperation.java 89f2ae9
service/src/java/org/apache/hive/service/cli/operation/Operation.java 3f36e2d
service/src/java/org/apache/hive/service/cli/operation/OperationManager.java 345617c
service/src/java/org/apache/hive/service/cli/operation/SQLOperation.java ace791a
Diff: https://reviews.apache.org/r/19395/diff/
Testing
-------
Thanks,
Vaibhav Gumashta
Re: Review Request 19395: JDBC cancel will not work with current HiveServer2
Posted by Vaibhav Gumashta <vg...@hortonworks.com>.
> On March 19, 2014, 3:01 p.m., Prasad Mujumdar wrote:
> > Looks fine overall. A couple of minor comments below.
> > Do we have a test for asynchronous cancel() ? You might want to log a followup ticket to add test cases.
I think it makes sense to add a test in this ticket itself. I did test it with a standalone jdbc client program, let me think of a good way to unit test it (maybe a sleep udf?).
> On March 19, 2014, 3:01 p.m., Prasad Mujumdar wrote:
> > service/src/java/org/apache/hive/service/cli/operation/OperationManager.java, line 163
> > <https://reviews.apache.org/r/19395/diff/1/?file=527980#file527980line163>
> >
> > Should the log message include operation handle as well ? Just the state won't be very helpful for analyzing logs ..
Sure, will do.
> On March 19, 2014, 3:01 p.m., Prasad Mujumdar wrote:
> > service/src/java/org/apache/hive/service/cli/operation/SQLOperation.java, line 150
> > <https://reviews.apache.org/r/19395/diff/1/?file=527981#file527981line150>
> >
> > As part of this patch, the client is already serializing cancel() as well as the cancelOperation() is validating the state before executing the cancel.
> > In that case, do we need this check again ?
The serialization within cancel (& elsewhere) was required since the same client socket is used for all the rpc calls during the session. However, this check is more for the thread running the execute call. In the case when a running operation is canceled by the other thread, we don't want the running thread to throw an error.
- Vaibhav
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19395/#review37713
-----------------------------------------------------------
On March 19, 2014, 11:02 a.m., Vaibhav Gumashta wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19395/
> -----------------------------------------------------------
>
> (Updated March 19, 2014, 11:02 a.m.)
>
>
> Review request for hive, Prasad Mujumdar and Thejas Nair.
>
>
> Bugs: HIVE-6472
> https://issues.apache.org/jira/browse/HIVE-6472
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> https://issues.apache.org/jira/browse/HIVE-6472
>
>
> Diffs
> -----
>
> jdbc/src/java/org/apache/hive/jdbc/HiveStatement.java 01e6ea7
> service/src/java/org/apache/hive/service/cli/OperationState.java a023908
> service/src/java/org/apache/hive/service/cli/operation/ExecuteStatementOperation.java 89f2ae9
> service/src/java/org/apache/hive/service/cli/operation/Operation.java 3f36e2d
> service/src/java/org/apache/hive/service/cli/operation/OperationManager.java 345617c
> service/src/java/org/apache/hive/service/cli/operation/SQLOperation.java ace791a
>
> Diff: https://reviews.apache.org/r/19395/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Vaibhav Gumashta
>
>
Re: Review Request 19395: JDBC cancel will not work with current HiveServer2
Posted by Prasad Mujumdar <pr...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19395/#review37713
-----------------------------------------------------------
Looks fine overall. A couple of minor comments below.
Do we have a test for asynchronous cancel() ? You might want to log a followup ticket to add test cases.
service/src/java/org/apache/hive/service/cli/operation/OperationManager.java
<https://reviews.apache.org/r/19395/#comment69359>
Should the log message include operation handle as well ? Just the state won't be very helpful for analyzing logs ..
service/src/java/org/apache/hive/service/cli/operation/SQLOperation.java
<https://reviews.apache.org/r/19395/#comment69360>
As part of this patch, the client is already serializing cancel() as well as the cancelOperation() is validating the state before executing the cancel.
In that case, do we need this check again ?
- Prasad Mujumdar
On March 19, 2014, 11:02 a.m., Vaibhav Gumashta wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19395/
> -----------------------------------------------------------
>
> (Updated March 19, 2014, 11:02 a.m.)
>
>
> Review request for hive, Prasad Mujumdar and Thejas Nair.
>
>
> Bugs: HIVE-6472
> https://issues.apache.org/jira/browse/HIVE-6472
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> https://issues.apache.org/jira/browse/HIVE-6472
>
>
> Diffs
> -----
>
> jdbc/src/java/org/apache/hive/jdbc/HiveStatement.java 01e6ea7
> service/src/java/org/apache/hive/service/cli/OperationState.java a023908
> service/src/java/org/apache/hive/service/cli/operation/ExecuteStatementOperation.java 89f2ae9
> service/src/java/org/apache/hive/service/cli/operation/Operation.java 3f36e2d
> service/src/java/org/apache/hive/service/cli/operation/OperationManager.java 345617c
> service/src/java/org/apache/hive/service/cli/operation/SQLOperation.java ace791a
>
> Diff: https://reviews.apache.org/r/19395/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Vaibhav Gumashta
>
>
Re: Review Request 19395: JDBC cancel will not work with current HiveServer2
Posted by Vaibhav Gumashta <vg...@hortonworks.com>.
> On March 20, 2014, 5:18 p.m., Prasad Mujumdar wrote:
> > Thanks for adding the test!
> > The patch look fine to me. Just wondering if the test could be a big flaky due to timings. We should keep an eye if it fails intermittently. It might help to set static flags to throttle the UDF execution.
Sure, if there are any issues, I'll do that. Thanks for the feedback!
- Vaibhav
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19395/#review37915
-----------------------------------------------------------
On March 20, 2014, 7:42 a.m., Vaibhav Gumashta wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19395/
> -----------------------------------------------------------
>
> (Updated March 20, 2014, 7:42 a.m.)
>
>
> Review request for hive, Jason Dere, Prasad Mujumdar, and Thejas Nair.
>
>
> Bugs: HIVE-6472
> https://issues.apache.org/jira/browse/HIVE-6472
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> https://issues.apache.org/jira/browse/HIVE-6472
>
>
> Diffs
> -----
>
> itests/hive-unit/src/test/java/org/apache/hive/jdbc/TestJdbcDriver2.java c91df83
> jdbc/src/java/org/apache/hive/jdbc/HiveStatement.java 01e6ea7
> service/src/java/org/apache/hive/service/cli/OperationState.java a023908
> service/src/java/org/apache/hive/service/cli/operation/ExecuteStatementOperation.java 89f2ae9
> service/src/java/org/apache/hive/service/cli/operation/Operation.java 3f36e2d
> service/src/java/org/apache/hive/service/cli/operation/OperationManager.java 345617c
> service/src/java/org/apache/hive/service/cli/operation/SQLOperation.java ace791a
>
> Diff: https://reviews.apache.org/r/19395/diff/
>
>
> Testing
> -------
>
> Added a new test to TestJdbcDriver2 and did testing with standalone jdbc program.
>
>
> Thanks,
>
> Vaibhav Gumashta
>
>
Re: Review Request 19395: JDBC cancel will not work with current HiveServer2
Posted by Prasad Mujumdar <pr...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19395/#review37915
-----------------------------------------------------------
Ship it!
Thanks for adding the test!
The patch look fine to me. Just wondering if the test could be a big flaky due to timings. We should keep an eye if it fails intermittently. It might help to set static flags to throttle the UDF execution.
- Prasad Mujumdar
On March 20, 2014, 7:42 a.m., Vaibhav Gumashta wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19395/
> -----------------------------------------------------------
>
> (Updated March 20, 2014, 7:42 a.m.)
>
>
> Review request for hive, Jason Dere, Prasad Mujumdar, and Thejas Nair.
>
>
> Bugs: HIVE-6472
> https://issues.apache.org/jira/browse/HIVE-6472
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> https://issues.apache.org/jira/browse/HIVE-6472
>
>
> Diffs
> -----
>
> itests/hive-unit/src/test/java/org/apache/hive/jdbc/TestJdbcDriver2.java c91df83
> jdbc/src/java/org/apache/hive/jdbc/HiveStatement.java 01e6ea7
> service/src/java/org/apache/hive/service/cli/OperationState.java a023908
> service/src/java/org/apache/hive/service/cli/operation/ExecuteStatementOperation.java 89f2ae9
> service/src/java/org/apache/hive/service/cli/operation/Operation.java 3f36e2d
> service/src/java/org/apache/hive/service/cli/operation/OperationManager.java 345617c
> service/src/java/org/apache/hive/service/cli/operation/SQLOperation.java ace791a
>
> Diff: https://reviews.apache.org/r/19395/diff/
>
>
> Testing
> -------
>
> Added a new test to TestJdbcDriver2 and did testing with standalone jdbc program.
>
>
> Thanks,
>
> Vaibhav Gumashta
>
>
Re: Review Request 19395: JDBC cancel will not work with current HiveServer2
Posted by Vaibhav Gumashta <vg...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19395/
-----------------------------------------------------------
(Updated March 20, 2014, 7:42 a.m.)
Review request for hive, Jason Dere, Prasad Mujumdar, and Thejas Nair.
Bugs: HIVE-6472
https://issues.apache.org/jira/browse/HIVE-6472
Repository: hive-git
Description
-------
https://issues.apache.org/jira/browse/HIVE-6472
Diffs
-----
itests/hive-unit/src/test/java/org/apache/hive/jdbc/TestJdbcDriver2.java c91df83
jdbc/src/java/org/apache/hive/jdbc/HiveStatement.java 01e6ea7
service/src/java/org/apache/hive/service/cli/OperationState.java a023908
service/src/java/org/apache/hive/service/cli/operation/ExecuteStatementOperation.java 89f2ae9
service/src/java/org/apache/hive/service/cli/operation/Operation.java 3f36e2d
service/src/java/org/apache/hive/service/cli/operation/OperationManager.java 345617c
service/src/java/org/apache/hive/service/cli/operation/SQLOperation.java ace791a
Diff: https://reviews.apache.org/r/19395/diff/
Testing
-------
Added a new test to TestJdbcDriver2 and did testing with standalone jdbc program.
Thanks,
Vaibhav Gumashta
Re: Review Request 19395: JDBC cancel will not work with current HiveServer2
Posted by Vaibhav Gumashta <vg...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19395/
-----------------------------------------------------------
(Updated March 20, 2014, 7:41 a.m.)
Review request for hive, Prasad Mujumdar and Thejas Nair.
Changes
-------
Addresses the feedback + improves client transport synchronization.
Bugs: HIVE-6472
https://issues.apache.org/jira/browse/HIVE-6472
Repository: hive-git
Description
-------
https://issues.apache.org/jira/browse/HIVE-6472
Diffs (updated)
-----
itests/hive-unit/src/test/java/org/apache/hive/jdbc/TestJdbcDriver2.java c91df83
jdbc/src/java/org/apache/hive/jdbc/HiveStatement.java 01e6ea7
service/src/java/org/apache/hive/service/cli/OperationState.java a023908
service/src/java/org/apache/hive/service/cli/operation/ExecuteStatementOperation.java 89f2ae9
service/src/java/org/apache/hive/service/cli/operation/Operation.java 3f36e2d
service/src/java/org/apache/hive/service/cli/operation/OperationManager.java 345617c
service/src/java/org/apache/hive/service/cli/operation/SQLOperation.java ace791a
Diff: https://reviews.apache.org/r/19395/diff/
Testing (updated)
-------
Added a new test to TestJdbcDriver2 and did testing with standalone jdbc program.
Thanks,
Vaibhav Gumashta