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 2013/11/01 01:54:36 UTC

Re: Review Request 15151: Better error reporting by async threads in HiveServer2

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

(Updated Nov. 1, 2013, 12:54 a.m.)


Review request for hive, Prasad Mujumdar and Thejas Nair.


Bugs: HIVE-5230
    https://issues.apache.org/jira/browse/HIVE-5230


Repository: hive-git


Description
-------

[HIVE-4617|https://issues.apache.org/jira/browse/HIVE-4617] provides support for async execution in HS2. When a background thread gets an error, currently the client can only poll for the operation state and also the error with its stacktrace is logged. However, it will be useful to provide a richer error response like thrift API does with TStatus (which is constructed while building a Thrift response object). 


Diffs
-----

  service/src/java/org/apache/hive/service/cli/CLIService.java 1a7f338 
  service/src/java/org/apache/hive/service/cli/CLIServiceClient.java 14ef54f 
  service/src/java/org/apache/hive/service/cli/EmbeddedCLIServiceClient.java 9dca874 
  service/src/java/org/apache/hive/service/cli/ICLIService.java f647ce6 
  service/src/java/org/apache/hive/service/cli/OperationStatus.java PRE-CREATION 
  service/src/java/org/apache/hive/service/cli/operation/Operation.java 6f4b8dc 
  service/src/java/org/apache/hive/service/cli/operation/OperationManager.java bcdb67f 
  service/src/java/org/apache/hive/service/cli/operation/SQLOperation.java f6adf92 
  service/src/java/org/apache/hive/service/cli/thrift/ThriftCLIService.java 9df110e 
  service/src/java/org/apache/hive/service/cli/thrift/ThriftCLIServiceClient.java 9bb2a0f 
  service/src/test/org/apache/hive/service/cli/CLIServiceTest.java d6caed1 
  service/src/test/org/apache/hive/service/cli/thrift/ThriftCLIServiceTest.java ff7166d 

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


Testing
-------


Thanks,

Vaibhav Gumashta

Re: Review Request 15151: Better error reporting by async threads in HiveServer2

Posted by Vaibhav Gumashta <vg...@hortonworks.com>.


> On Nov. 5, 2013, 3:32 a.m., Thejas Nair wrote:
> > service/src/java/org/apache/hive/service/cli/operation/SQLOperation.java, line 70
> > <https://reviews.apache.org/r/15151/diff/1/?file=375372#file375372line70>
> >
> >     This and some other variables used by async execution needs to be made volatile, as it can now be accessed from multiple threads. We should also do that for state in Operation.java . But that can be addressed in another thread.
> >
> 
> Vaibhav Gumashta wrote:
>     It seems we might not need to make this volatile. From the java docs (http://docs.oracle.com/javase/7/docs/api/java/util/concurrent/Executor.html): Memory consistency effects: Actions in a thread prior to submitting a Runnable object to an Executor happen-before its execution begins, perhaps in another thread. This implies that backgroundHandle will always be set to null before the execution of a new runnable is started in any of the workers which were previously handling some other runnable.
> 
> Thejas Nair wrote:
>     Consider the case of doing a execute followed by a getstatus call from client. The execute statement say gets run by thread x and getstatus gets run by thread y. The executor creates a happens before relation only between statements in thread x and the start of the async thread. The assignment of backgroundHandle value is a step after the Executor.execute call. So it does not establish a happens-before relationship between the assignment of backgroundHandle value in thread x with the lookup for backgroundHandle in thread y.
>
> 
> Thejas Nair wrote:
>     Thinking some more about it, I think it would be safer to establish a happens-before relationship at higher level in HiveSessionImpl calls instead.
>

Thinking more about it, I agree. Especially if x and y run on different cores (p1, p2), even though y is called after x has finished execution (GetOperationStatus always called after Execute), the value of backgroundHandle in p1's cache written by x, might not have propagated to p2. Declaring backgroundHandle as volatile will successfully invalidate it in all other caches when x writes, and thus thread y on p2 will always get a cache miss and read the latest value.


- Vaibhav


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


On Nov. 1, 2013, 12:54 a.m., Vaibhav Gumashta wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15151/
> -----------------------------------------------------------
> 
> (Updated Nov. 1, 2013, 12:54 a.m.)
> 
> 
> Review request for hive, Prasad Mujumdar and Thejas Nair.
> 
> 
> Bugs: HIVE-5230
>     https://issues.apache.org/jira/browse/HIVE-5230
> 
> 
> Repository: hive-git
> 
> 
> Description
> -------
> 
> [HIVE-4617|https://issues.apache.org/jira/browse/HIVE-4617] provides support for async execution in HS2. When a background thread gets an error, currently the client can only poll for the operation state and also the error with its stacktrace is logged. However, it will be useful to provide a richer error response like thrift API does with TStatus (which is constructed while building a Thrift response object). 
> 
> 
> Diffs
> -----
> 
>   service/src/java/org/apache/hive/service/cli/CLIService.java 1a7f338 
>   service/src/java/org/apache/hive/service/cli/CLIServiceClient.java 14ef54f 
>   service/src/java/org/apache/hive/service/cli/EmbeddedCLIServiceClient.java 9dca874 
>   service/src/java/org/apache/hive/service/cli/ICLIService.java f647ce6 
>   service/src/java/org/apache/hive/service/cli/OperationStatus.java PRE-CREATION 
>   service/src/java/org/apache/hive/service/cli/operation/Operation.java 6f4b8dc 
>   service/src/java/org/apache/hive/service/cli/operation/OperationManager.java bcdb67f 
>   service/src/java/org/apache/hive/service/cli/operation/SQLOperation.java f6adf92 
>   service/src/java/org/apache/hive/service/cli/thrift/ThriftCLIService.java 9df110e 
>   service/src/java/org/apache/hive/service/cli/thrift/ThriftCLIServiceClient.java 9bb2a0f 
>   service/src/test/org/apache/hive/service/cli/CLIServiceTest.java d6caed1 
>   service/src/test/org/apache/hive/service/cli/thrift/ThriftCLIServiceTest.java ff7166d 
> 
> Diff: https://reviews.apache.org/r/15151/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Vaibhav Gumashta
> 
>

Re: Review Request 15151: Better error reporting by async threads in HiveServer2

Posted by Thejas Nair <th...@hortonworks.com>.


> On Nov. 5, 2013, 3:32 a.m., Thejas Nair wrote:
> > service/src/java/org/apache/hive/service/cli/operation/SQLOperation.java, line 70
> > <https://reviews.apache.org/r/15151/diff/1/?file=375372#file375372line70>
> >
> >     This and some other variables used by async execution needs to be made volatile, as it can now be accessed from multiple threads. We should also do that for state in Operation.java . But that can be addressed in another thread.
> >
> 
> Vaibhav Gumashta wrote:
>     It seems we might not need to make this volatile. From the java docs (http://docs.oracle.com/javase/7/docs/api/java/util/concurrent/Executor.html): Memory consistency effects: Actions in a thread prior to submitting a Runnable object to an Executor happen-before its execution begins, perhaps in another thread. This implies that backgroundHandle will always be set to null before the execution of a new runnable is started in any of the workers which were previously handling some other runnable.
> 
> Thejas Nair wrote:
>     Consider the case of doing a execute followed by a getstatus call from client. The execute statement say gets run by thread x and getstatus gets run by thread y. The executor creates a happens before relation only between statements in thread x and the start of the async thread. The assignment of backgroundHandle value is a step after the Executor.execute call. So it does not establish a happens-before relationship between the assignment of backgroundHandle value in thread x with the lookup for backgroundHandle in thread y.
>

Thinking some more about it, I think it would be safer to establish a happens-before relationship at higher level in HiveSessionImpl calls instead.


- Thejas


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


On Nov. 1, 2013, 12:54 a.m., Vaibhav Gumashta wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15151/
> -----------------------------------------------------------
> 
> (Updated Nov. 1, 2013, 12:54 a.m.)
> 
> 
> Review request for hive, Prasad Mujumdar and Thejas Nair.
> 
> 
> Bugs: HIVE-5230
>     https://issues.apache.org/jira/browse/HIVE-5230
> 
> 
> Repository: hive-git
> 
> 
> Description
> -------
> 
> [HIVE-4617|https://issues.apache.org/jira/browse/HIVE-4617] provides support for async execution in HS2. When a background thread gets an error, currently the client can only poll for the operation state and also the error with its stacktrace is logged. However, it will be useful to provide a richer error response like thrift API does with TStatus (which is constructed while building a Thrift response object). 
> 
> 
> Diffs
> -----
> 
>   service/src/java/org/apache/hive/service/cli/CLIService.java 1a7f338 
>   service/src/java/org/apache/hive/service/cli/CLIServiceClient.java 14ef54f 
>   service/src/java/org/apache/hive/service/cli/EmbeddedCLIServiceClient.java 9dca874 
>   service/src/java/org/apache/hive/service/cli/ICLIService.java f647ce6 
>   service/src/java/org/apache/hive/service/cli/OperationStatus.java PRE-CREATION 
>   service/src/java/org/apache/hive/service/cli/operation/Operation.java 6f4b8dc 
>   service/src/java/org/apache/hive/service/cli/operation/OperationManager.java bcdb67f 
>   service/src/java/org/apache/hive/service/cli/operation/SQLOperation.java f6adf92 
>   service/src/java/org/apache/hive/service/cli/thrift/ThriftCLIService.java 9df110e 
>   service/src/java/org/apache/hive/service/cli/thrift/ThriftCLIServiceClient.java 9bb2a0f 
>   service/src/test/org/apache/hive/service/cli/CLIServiceTest.java d6caed1 
>   service/src/test/org/apache/hive/service/cli/thrift/ThriftCLIServiceTest.java ff7166d 
> 
> Diff: https://reviews.apache.org/r/15151/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Vaibhav Gumashta
> 
>

Re: Review Request 15151: Better error reporting by async threads in HiveServer2

Posted by Vaibhav Gumashta <vg...@hortonworks.com>.


> On Nov. 5, 2013, 3:32 a.m., Thejas Nair wrote:
> > service/src/java/org/apache/hive/service/cli/operation/SQLOperation.java, line 70
> > <https://reviews.apache.org/r/15151/diff/1/?file=375372#file375372line70>
> >
> >     This and some other variables used by async execution needs to be made volatile, as it can now be accessed from multiple threads. We should also do that for state in Operation.java . But that can be addressed in another thread.
> >

It seems we might not need to make this volatile. From the java docs (http://docs.oracle.com/javase/7/docs/api/java/util/concurrent/Executor.html): Memory consistency effects: Actions in a thread prior to submitting a Runnable object to an Executor happen-before its execution begins, perhaps in another thread. This implies that backgroundHandle will always be set to null before the execution of a new runnable is started in any of the workers which were previously handling some other runnable.


> On Nov. 5, 2013, 3:32 a.m., Thejas Nair wrote:
> > service/src/java/org/apache/hive/service/cli/operation/SQLOperation.java, line 71
> > <https://reviews.apache.org/r/15151/diff/1/?file=375372#file375372line71>
> >
> >     we should make this volatile. This can be accessed from multiple threads.
> >

It seems even this will be set to null before a worker starts the new runnable.


> On Nov. 5, 2013, 3:32 a.m., Thejas Nair wrote:
> > service/src/java/org/apache/hive/service/cli/thrift/ThriftCLIServiceClient.java, line 304
> > <https://reviews.apache.org/r/15151/diff/1/?file=375374#file375374line304>
> >
> >     why is it returning null instead of the exception ?

Since this has the same design as ICLIService#getOperationStatus, in case of error it is supposed to throw an exception. checkStatus does that and if the flow reaches here, that means there is no exception to throw.


> On Nov. 5, 2013, 3:32 a.m., Thejas Nair wrote:
> > service/src/test/org/apache/hive/service/cli/CLIServiceTest.java, line 187
> > <https://reviews.apache.org/r/15151/diff/1/?file=375375#file375375line187>
> >
> >     assertEquals("operation state", OperationState.ERROR, state) will give a more informative error if test fails.
> >

Sure, will make the change.


> On Nov. 5, 2013, 3:32 a.m., Thejas Nair wrote:
> > service/src/test/org/apache/hive/service/cli/CLIServiceTest.java, line 190
> > <https://reviews.apache.org/r/15151/diff/1/?file=375375#file375375line190>
> >
> >     can you also add a check for the error message ?

I was under the impression that the error message which are generated from org.apache.hadoop.hive.ql.ErrorMsg are subject to change. What do you think?


> On Nov. 5, 2013, 3:32 a.m., Thejas Nair wrote:
> > service/src/test/org/apache/hive/service/cli/thrift/ThriftCLIServiceTest.java, line 309
> > <https://reviews.apache.org/r/15151/diff/1/?file=375376#file375376line309>
> >
> >     can you add a check on the error message string here ?

Same as above. Let me know if you think otherwise, I can add the check.


- Vaibhav


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


On Nov. 1, 2013, 12:54 a.m., Vaibhav Gumashta wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15151/
> -----------------------------------------------------------
> 
> (Updated Nov. 1, 2013, 12:54 a.m.)
> 
> 
> Review request for hive, Prasad Mujumdar and Thejas Nair.
> 
> 
> Bugs: HIVE-5230
>     https://issues.apache.org/jira/browse/HIVE-5230
> 
> 
> Repository: hive-git
> 
> 
> Description
> -------
> 
> [HIVE-4617|https://issues.apache.org/jira/browse/HIVE-4617] provides support for async execution in HS2. When a background thread gets an error, currently the client can only poll for the operation state and also the error with its stacktrace is logged. However, it will be useful to provide a richer error response like thrift API does with TStatus (which is constructed while building a Thrift response object). 
> 
> 
> Diffs
> -----
> 
>   service/src/java/org/apache/hive/service/cli/CLIService.java 1a7f338 
>   service/src/java/org/apache/hive/service/cli/CLIServiceClient.java 14ef54f 
>   service/src/java/org/apache/hive/service/cli/EmbeddedCLIServiceClient.java 9dca874 
>   service/src/java/org/apache/hive/service/cli/ICLIService.java f647ce6 
>   service/src/java/org/apache/hive/service/cli/OperationStatus.java PRE-CREATION 
>   service/src/java/org/apache/hive/service/cli/operation/Operation.java 6f4b8dc 
>   service/src/java/org/apache/hive/service/cli/operation/OperationManager.java bcdb67f 
>   service/src/java/org/apache/hive/service/cli/operation/SQLOperation.java f6adf92 
>   service/src/java/org/apache/hive/service/cli/thrift/ThriftCLIService.java 9df110e 
>   service/src/java/org/apache/hive/service/cli/thrift/ThriftCLIServiceClient.java 9bb2a0f 
>   service/src/test/org/apache/hive/service/cli/CLIServiceTest.java d6caed1 
>   service/src/test/org/apache/hive/service/cli/thrift/ThriftCLIServiceTest.java ff7166d 
> 
> Diff: https://reviews.apache.org/r/15151/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Vaibhav Gumashta
> 
>

Re: Review Request 15151: Better error reporting by async threads in HiveServer2

Posted by Thejas Nair <th...@hortonworks.com>.


> On Nov. 5, 2013, 3:32 a.m., Thejas Nair wrote:
> > service/src/java/org/apache/hive/service/cli/operation/SQLOperation.java, line 70
> > <https://reviews.apache.org/r/15151/diff/1/?file=375372#file375372line70>
> >
> >     This and some other variables used by async execution needs to be made volatile, as it can now be accessed from multiple threads. We should also do that for state in Operation.java . But that can be addressed in another thread.
> >
> 
> Vaibhav Gumashta wrote:
>     It seems we might not need to make this volatile. From the java docs (http://docs.oracle.com/javase/7/docs/api/java/util/concurrent/Executor.html): Memory consistency effects: Actions in a thread prior to submitting a Runnable object to an Executor happen-before its execution begins, perhaps in another thread. This implies that backgroundHandle will always be set to null before the execution of a new runnable is started in any of the workers which were previously handling some other runnable.

Consider the case of doing a execute followed by a getstatus call from client. The execute statement say gets run by thread x and getstatus gets run by thread y. The executor creates a happens before relation only between statements in thread x and the start of the async thread. The assignment of backgroundHandle value is a step after the Executor.execute call. So it does not establish a happens-before relationship between the assignment of backgroundHandle value in thread x with the lookup for backgroundHandle in thread y.


> On Nov. 5, 2013, 3:32 a.m., Thejas Nair wrote:
> > service/src/java/org/apache/hive/service/cli/operation/SQLOperation.java, line 71
> > <https://reviews.apache.org/r/15151/diff/1/?file=375372#file375372line71>
> >
> >     we should make this volatile. This can be accessed from multiple threads.
> >
> 
> Vaibhav Gumashta wrote:
>     It seems even this will be set to null before a worker starts the new runnable.

Yes, it will be null before a worker starts new runnable. But when that async runnable sets the runException, another thread that is processing the getStatus call might still see it as null.


- Thejas


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


On Nov. 1, 2013, 12:54 a.m., Vaibhav Gumashta wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15151/
> -----------------------------------------------------------
> 
> (Updated Nov. 1, 2013, 12:54 a.m.)
> 
> 
> Review request for hive, Prasad Mujumdar and Thejas Nair.
> 
> 
> Bugs: HIVE-5230
>     https://issues.apache.org/jira/browse/HIVE-5230
> 
> 
> Repository: hive-git
> 
> 
> Description
> -------
> 
> [HIVE-4617|https://issues.apache.org/jira/browse/HIVE-4617] provides support for async execution in HS2. When a background thread gets an error, currently the client can only poll for the operation state and also the error with its stacktrace is logged. However, it will be useful to provide a richer error response like thrift API does with TStatus (which is constructed while building a Thrift response object). 
> 
> 
> Diffs
> -----
> 
>   service/src/java/org/apache/hive/service/cli/CLIService.java 1a7f338 
>   service/src/java/org/apache/hive/service/cli/CLIServiceClient.java 14ef54f 
>   service/src/java/org/apache/hive/service/cli/EmbeddedCLIServiceClient.java 9dca874 
>   service/src/java/org/apache/hive/service/cli/ICLIService.java f647ce6 
>   service/src/java/org/apache/hive/service/cli/OperationStatus.java PRE-CREATION 
>   service/src/java/org/apache/hive/service/cli/operation/Operation.java 6f4b8dc 
>   service/src/java/org/apache/hive/service/cli/operation/OperationManager.java bcdb67f 
>   service/src/java/org/apache/hive/service/cli/operation/SQLOperation.java f6adf92 
>   service/src/java/org/apache/hive/service/cli/thrift/ThriftCLIService.java 9df110e 
>   service/src/java/org/apache/hive/service/cli/thrift/ThriftCLIServiceClient.java 9bb2a0f 
>   service/src/test/org/apache/hive/service/cli/CLIServiceTest.java d6caed1 
>   service/src/test/org/apache/hive/service/cli/thrift/ThriftCLIServiceTest.java ff7166d 
> 
> Diff: https://reviews.apache.org/r/15151/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Vaibhav Gumashta
> 
>

Re: Review Request 15151: Better error reporting by async threads in HiveServer2

Posted by Thejas Nair <th...@hortonworks.com>.

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



service/src/java/org/apache/hive/service/cli/operation/SQLOperation.java
<https://reviews.apache.org/r/15151/#comment54799>

    This and some other variables used by async execution needs to be made volatile, as it can now be accessed from multiple threads. We should also do that for state in Operation.java . But that can be addressed in another thread.
    



service/src/java/org/apache/hive/service/cli/operation/SQLOperation.java
<https://reviews.apache.org/r/15151/#comment54798>

    we should make this volatile. This can be accessed from multiple threads.
    



service/src/java/org/apache/hive/service/cli/thrift/ThriftCLIServiceClient.java
<https://reviews.apache.org/r/15151/#comment54800>

    why is it returning null instead of the exception ?



service/src/test/org/apache/hive/service/cli/CLIServiceTest.java
<https://reviews.apache.org/r/15151/#comment54802>

    assertEquals("operation state", OperationState.ERROR, state) will give a more informative error if test fails.
    



service/src/test/org/apache/hive/service/cli/CLIServiceTest.java
<https://reviews.apache.org/r/15151/#comment54801>

    can you also add a check for the error message ?



service/src/test/org/apache/hive/service/cli/thrift/ThriftCLIServiceTest.java
<https://reviews.apache.org/r/15151/#comment54803>

    can you add a check on the error message string here ?


- Thejas Nair


On Nov. 1, 2013, 12:54 a.m., Vaibhav Gumashta wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15151/
> -----------------------------------------------------------
> 
> (Updated Nov. 1, 2013, 12:54 a.m.)
> 
> 
> Review request for hive, Prasad Mujumdar and Thejas Nair.
> 
> 
> Bugs: HIVE-5230
>     https://issues.apache.org/jira/browse/HIVE-5230
> 
> 
> Repository: hive-git
> 
> 
> Description
> -------
> 
> [HIVE-4617|https://issues.apache.org/jira/browse/HIVE-4617] provides support for async execution in HS2. When a background thread gets an error, currently the client can only poll for the operation state and also the error with its stacktrace is logged. However, it will be useful to provide a richer error response like thrift API does with TStatus (which is constructed while building a Thrift response object). 
> 
> 
> Diffs
> -----
> 
>   service/src/java/org/apache/hive/service/cli/CLIService.java 1a7f338 
>   service/src/java/org/apache/hive/service/cli/CLIServiceClient.java 14ef54f 
>   service/src/java/org/apache/hive/service/cli/EmbeddedCLIServiceClient.java 9dca874 
>   service/src/java/org/apache/hive/service/cli/ICLIService.java f647ce6 
>   service/src/java/org/apache/hive/service/cli/OperationStatus.java PRE-CREATION 
>   service/src/java/org/apache/hive/service/cli/operation/Operation.java 6f4b8dc 
>   service/src/java/org/apache/hive/service/cli/operation/OperationManager.java bcdb67f 
>   service/src/java/org/apache/hive/service/cli/operation/SQLOperation.java f6adf92 
>   service/src/java/org/apache/hive/service/cli/thrift/ThriftCLIService.java 9df110e 
>   service/src/java/org/apache/hive/service/cli/thrift/ThriftCLIServiceClient.java 9bb2a0f 
>   service/src/test/org/apache/hive/service/cli/CLIServiceTest.java d6caed1 
>   service/src/test/org/apache/hive/service/cli/thrift/ThriftCLIServiceTest.java ff7166d 
> 
> Diff: https://reviews.apache.org/r/15151/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Vaibhav Gumashta
> 
>

Re: Review Request 15151: Better error reporting by async threads in HiveServer2

Posted by Vaibhav Gumashta <vg...@hortonworks.com>.


> On Nov. 8, 2013, 8:33 p.m., Prasad Mujumdar wrote:
> > @Vaibhav, thanks for taking the issue forward and putting a new patch!
> > 
> > I do have a high level comment on the approach. The 'status' returned by HS2 RPC is suppose to be the status of that particular API's execution. Where as in this case, we are overloading the 'status' field to return the status of a different (ie. ExecuteStatement()) status.
> > For example, if you call GetStatus() with a non-existing operation id then you would get an error status. This error is for failure of the GetStatus() itself. On the other hand if you call GetStatus() for an async query that failed, then you will also get the error status. However this error is not for the current GetStatus() operation, but for the last ExecuteStatement() operation.
> > The current implementation of the JDBC driver (or CLIClient in general) will work with this, but perhaps its not a clean way to implement it. Have you considered adding a new field in the GetStatus response to return the error status of the actual execute operation ?
> > 
> >

Thanks for pointing out the design intent Prasad. I agree, expanding the API to include the operation error in GetStatus response is a cleaner implementation. I've made the relevant changes. Let me know if you have any feedback on the latest patch. Thanks! 


- Vaibhav


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


On Nov. 11, 2013, 7:23 p.m., Vaibhav Gumashta wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15151/
> -----------------------------------------------------------
> 
> (Updated Nov. 11, 2013, 7:23 p.m.)
> 
> 
> Review request for hive, Prasad Mujumdar and Thejas Nair.
> 
> 
> Bugs: HIVE-5230
>     https://issues.apache.org/jira/browse/HIVE-5230
> 
> 
> Repository: hive-git
> 
> 
> Description
> -------
> 
> [HIVE-4617|https://issues.apache.org/jira/browse/HIVE-4617] provides support for async execution in HS2. When a background thread gets an error, currently the client can only poll for the operation state and also the error with its stacktrace is logged. However, it will be useful to provide a richer error response like thrift API does with TStatus (which is constructed while building a Thrift response object). 
> 
> 
> Diffs
> -----
> 
>   itests/hive-unit/src/test/java/org/apache/hive/service/cli/CLIServiceTest.java PRE-CREATION 
>   itests/hive-unit/src/test/java/org/apache/hive/service/cli/thrift/ThriftCLIServiceTest.java PRE-CREATION 
>   jdbc/src/java/org/apache/hive/jdbc/HiveStatement.java fce19bf 
>   service/if/TCLIService.thrift 1f49445 
>   service/src/java/org/apache/hive/service/cli/CLIService.java 8c85386 
>   service/src/java/org/apache/hive/service/cli/CLIServiceClient.java 14ef54f 
>   service/src/java/org/apache/hive/service/cli/EmbeddedCLIServiceClient.java 9dca874 
>   service/src/java/org/apache/hive/service/cli/HiveSQLException.java 74e8b94 
>   service/src/java/org/apache/hive/service/cli/ICLIService.java f647ce6 
>   service/src/java/org/apache/hive/service/cli/OperationState.java 1ec6bd1 
>   service/src/java/org/apache/hive/service/cli/OperationStatus.java PRE-CREATION 
>   service/src/java/org/apache/hive/service/cli/operation/Operation.java 6f4b8dc 
>   service/src/java/org/apache/hive/service/cli/operation/OperationManager.java bcdb67f 
>   service/src/java/org/apache/hive/service/cli/operation/SQLOperation.java 4ee1b74 
>   service/src/java/org/apache/hive/service/cli/thrift/ThriftCLIService.java 9df110e 
>   service/src/java/org/apache/hive/service/cli/thrift/ThriftCLIServiceClient.java 9bb2a0f 
>   service/src/test/org/apache/hive/service/cli/CLIServiceTest.java cd9d99a 
>   service/src/test/org/apache/hive/service/cli/thrift/ThriftCLIServiceTest.java ff7166d 
> 
> Diff: https://reviews.apache.org/r/15151/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Vaibhav Gumashta
> 
>

Re: Review Request 15151: Better error reporting by async threads in HiveServer2

Posted by Prasad Mujumdar <pr...@cloudera.com>.

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


@Vaibhav, thanks for taking the issue forward and putting a new patch!

I do have a high level comment on the approach. The 'status' returned by HS2 RPC is suppose to be the status of that particular API's execution. Where as in this case, we are overloading the 'status' field to return the status of a different (ie. ExecuteStatement()) status.
For example, if you call GetStatus() with a non-existing operation id then you would get an error status. This error is for failure of the GetStatus() itself. On the other hand if you call GetStatus() for an async query that failed, then you will also get the error status. However this error is not for the current GetStatus() operation, but for the last ExecuteStatement() operation.
The current implementation of the JDBC driver (or CLIClient in general) will work with this, but perhaps its not a clean way to implement it. Have you considered adding a new field in the GetStatus response to return the error status of the actual execute operation ?



- Prasad Mujumdar


On Nov. 1, 2013, 12:54 a.m., Vaibhav Gumashta wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15151/
> -----------------------------------------------------------
> 
> (Updated Nov. 1, 2013, 12:54 a.m.)
> 
> 
> Review request for hive, Prasad Mujumdar and Thejas Nair.
> 
> 
> Bugs: HIVE-5230
>     https://issues.apache.org/jira/browse/HIVE-5230
> 
> 
> Repository: hive-git
> 
> 
> Description
> -------
> 
> [HIVE-4617|https://issues.apache.org/jira/browse/HIVE-4617] provides support for async execution in HS2. When a background thread gets an error, currently the client can only poll for the operation state and also the error with its stacktrace is logged. However, it will be useful to provide a richer error response like thrift API does with TStatus (which is constructed while building a Thrift response object). 
> 
> 
> Diffs
> -----
> 
>   service/src/java/org/apache/hive/service/cli/CLIService.java 1a7f338 
>   service/src/java/org/apache/hive/service/cli/CLIServiceClient.java 14ef54f 
>   service/src/java/org/apache/hive/service/cli/EmbeddedCLIServiceClient.java 9dca874 
>   service/src/java/org/apache/hive/service/cli/ICLIService.java f647ce6 
>   service/src/java/org/apache/hive/service/cli/OperationStatus.java PRE-CREATION 
>   service/src/java/org/apache/hive/service/cli/operation/Operation.java 6f4b8dc 
>   service/src/java/org/apache/hive/service/cli/operation/OperationManager.java bcdb67f 
>   service/src/java/org/apache/hive/service/cli/operation/SQLOperation.java f6adf92 
>   service/src/java/org/apache/hive/service/cli/thrift/ThriftCLIService.java 9df110e 
>   service/src/java/org/apache/hive/service/cli/thrift/ThriftCLIServiceClient.java 9bb2a0f 
>   service/src/test/org/apache/hive/service/cli/CLIServiceTest.java d6caed1 
>   service/src/test/org/apache/hive/service/cli/thrift/ThriftCLIServiceTest.java ff7166d 
> 
> Diff: https://reviews.apache.org/r/15151/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Vaibhav Gumashta
> 
>

Re: Review Request 15151: Better error reporting by async threads in HiveServer2

Posted by Vaibhav Gumashta <vg...@hortonworks.com>.


> On Nov. 11, 2013, 7:54 p.m., Thejas Nair wrote:
> > service/if/TCLIService.thrift, line 917
> > <https://reviews.apache.org/r/15151/diff/4/?file=382149#file382149line917>
> >
> >     This will not be backward compatible. It would be better to add a new optional field in the response, that has the additional error information.
> >

Fixed. Thanks for pointing out!


- Vaibhav


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


On Nov. 11, 2013, 7:23 p.m., Vaibhav Gumashta wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15151/
> -----------------------------------------------------------
> 
> (Updated Nov. 11, 2013, 7:23 p.m.)
> 
> 
> Review request for hive, Prasad Mujumdar and Thejas Nair.
> 
> 
> Bugs: HIVE-5230
>     https://issues.apache.org/jira/browse/HIVE-5230
> 
> 
> Repository: hive-git
> 
> 
> Description
> -------
> 
> [HIVE-4617|https://issues.apache.org/jira/browse/HIVE-4617] provides support for async execution in HS2. When a background thread gets an error, currently the client can only poll for the operation state and also the error with its stacktrace is logged. However, it will be useful to provide a richer error response like thrift API does with TStatus (which is constructed while building a Thrift response object). 
> 
> 
> Diffs
> -----
> 
>   itests/hive-unit/src/test/java/org/apache/hive/service/cli/CLIServiceTest.java PRE-CREATION 
>   itests/hive-unit/src/test/java/org/apache/hive/service/cli/thrift/ThriftCLIServiceTest.java PRE-CREATION 
>   jdbc/src/java/org/apache/hive/jdbc/HiveStatement.java fce19bf 
>   service/if/TCLIService.thrift 1f49445 
>   service/src/java/org/apache/hive/service/cli/CLIService.java 8c85386 
>   service/src/java/org/apache/hive/service/cli/CLIServiceClient.java 14ef54f 
>   service/src/java/org/apache/hive/service/cli/EmbeddedCLIServiceClient.java 9dca874 
>   service/src/java/org/apache/hive/service/cli/HiveSQLException.java 74e8b94 
>   service/src/java/org/apache/hive/service/cli/ICLIService.java f647ce6 
>   service/src/java/org/apache/hive/service/cli/OperationState.java 1ec6bd1 
>   service/src/java/org/apache/hive/service/cli/OperationStatus.java PRE-CREATION 
>   service/src/java/org/apache/hive/service/cli/operation/Operation.java 6f4b8dc 
>   service/src/java/org/apache/hive/service/cli/operation/OperationManager.java bcdb67f 
>   service/src/java/org/apache/hive/service/cli/operation/SQLOperation.java 4ee1b74 
>   service/src/java/org/apache/hive/service/cli/thrift/ThriftCLIService.java 9df110e 
>   service/src/java/org/apache/hive/service/cli/thrift/ThriftCLIServiceClient.java 9bb2a0f 
>   service/src/test/org/apache/hive/service/cli/CLIServiceTest.java cd9d99a 
>   service/src/test/org/apache/hive/service/cli/thrift/ThriftCLIServiceTest.java ff7166d 
> 
> Diff: https://reviews.apache.org/r/15151/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Vaibhav Gumashta
> 
>

Re: Review Request 15151: Better error reporting by async threads in HiveServer2

Posted by Thejas Nair <th...@hortonworks.com>.

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



service/if/TCLIService.thrift
<https://reviews.apache.org/r/15151/#comment55634>

    This will not be backward compatible. It would be better to add a new optional field in the response, that has the additional error information.
    


- Thejas Nair


On Nov. 11, 2013, 7:23 p.m., Vaibhav Gumashta wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15151/
> -----------------------------------------------------------
> 
> (Updated Nov. 11, 2013, 7:23 p.m.)
> 
> 
> Review request for hive, Prasad Mujumdar and Thejas Nair.
> 
> 
> Bugs: HIVE-5230
>     https://issues.apache.org/jira/browse/HIVE-5230
> 
> 
> Repository: hive-git
> 
> 
> Description
> -------
> 
> [HIVE-4617|https://issues.apache.org/jira/browse/HIVE-4617] provides support for async execution in HS2. When a background thread gets an error, currently the client can only poll for the operation state and also the error with its stacktrace is logged. However, it will be useful to provide a richer error response like thrift API does with TStatus (which is constructed while building a Thrift response object). 
> 
> 
> Diffs
> -----
> 
>   itests/hive-unit/src/test/java/org/apache/hive/service/cli/CLIServiceTest.java PRE-CREATION 
>   itests/hive-unit/src/test/java/org/apache/hive/service/cli/thrift/ThriftCLIServiceTest.java PRE-CREATION 
>   jdbc/src/java/org/apache/hive/jdbc/HiveStatement.java fce19bf 
>   service/if/TCLIService.thrift 1f49445 
>   service/src/java/org/apache/hive/service/cli/CLIService.java 8c85386 
>   service/src/java/org/apache/hive/service/cli/CLIServiceClient.java 14ef54f 
>   service/src/java/org/apache/hive/service/cli/EmbeddedCLIServiceClient.java 9dca874 
>   service/src/java/org/apache/hive/service/cli/HiveSQLException.java 74e8b94 
>   service/src/java/org/apache/hive/service/cli/ICLIService.java f647ce6 
>   service/src/java/org/apache/hive/service/cli/OperationState.java 1ec6bd1 
>   service/src/java/org/apache/hive/service/cli/OperationStatus.java PRE-CREATION 
>   service/src/java/org/apache/hive/service/cli/operation/Operation.java 6f4b8dc 
>   service/src/java/org/apache/hive/service/cli/operation/OperationManager.java bcdb67f 
>   service/src/java/org/apache/hive/service/cli/operation/SQLOperation.java 4ee1b74 
>   service/src/java/org/apache/hive/service/cli/thrift/ThriftCLIService.java 9df110e 
>   service/src/java/org/apache/hive/service/cli/thrift/ThriftCLIServiceClient.java 9bb2a0f 
>   service/src/test/org/apache/hive/service/cli/CLIServiceTest.java cd9d99a 
>   service/src/test/org/apache/hive/service/cli/thrift/ThriftCLIServiceTest.java ff7166d 
> 
> Diff: https://reviews.apache.org/r/15151/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Vaibhav Gumashta
> 
>

Re: Review Request 15151: Better error reporting by async threads in HiveServer2

Posted by Vaibhav Gumashta <vg...@hortonworks.com>.


> On Nov. 11, 2013, 7:52 p.m., Carl Steinbach wrote:
> > service/if/TCLIService.thrift, line 395
> > <https://reviews.apache.org/r/15151/diff/3/?file=381909#file381909line395>
> >
> >     Please reuse TStatus instead of adding a new struct.

My understanding is that TStatus encapsulates the status of an RPC request (as pointed out by Prasad earlier). It has a required TStatusCode field as well which corresponds to the status of an RPC call. Would it not be better to expand TGetOperationStatusResp with sqlState, errorCode and errorMessage?


> On Nov. 11, 2013, 7:52 p.m., Carl Steinbach wrote:
> > itests/hive-unit/src/test/java/org/apache/hive/service/cli/CLIServiceTest.java, line 1
> > <https://reviews.apache.org/r/15151/diff/3/?file=381906#file381906line1>
> >
> >     Does this patch make any changes to CLIServiceTest or ThriftCLIServiceTest, or does it just move these files from service/ to itests/ ? If it does make changes can we move the files in a different patch?

It moves these files, but also adds test for this feature + some minor refactoring.


> On Nov. 11, 2013, 7:52 p.m., Carl Steinbach wrote:
> > service/if/TCLIService.thrift, line 50
> > <https://reviews.apache.org/r/15151/diff/3/?file=381909#file381909line50>
> >
> >     Need to add HIVE_CLI_SERVICE_PROTOCOL_V5 and update any references to HIVE_CLI_SERVICE_PROTOCOL_V4.

Done


> On Nov. 11, 2013, 7:52 p.m., Carl Steinbach wrote:
> > service/src/java/org/apache/hive/service/cli/CLIService.java, line 274
> > <https://reviews.apache.org/r/15151/diff/3/?file=381910#file381910line274>
> >
> >     Please push this logic into OperationManager.getOperationStatus()

Done


> On Nov. 11, 2013, 7:52 p.m., Carl Steinbach wrote:
> > service/if/TCLIService.thrift, line 917
> > <https://reviews.apache.org/r/15151/diff/3/?file=381909#file381909line917>
> >
> >     This modification will break compatibility between upversion/downversion clients and servers since it modifies the type of an existing field. 
> >     
> >     It's possible to avoid this problem by instead adding a new "TStatus operationStatus" field.

Sorry about the slip, will expand TGetOperationStatusResp with a new optional field(s). Would appreciate your thoughts on my reply in the previous comment regarding TStatus.


> On Nov. 11, 2013, 7:52 p.m., Carl Steinbach wrote:
> > service/src/java/org/apache/hive/service/cli/operation/OperationManager.java, line 147
> > <https://reviews.apache.org/r/15151/diff/3/?file=381918#file381918line147>
> >
> >     s/getOperationRunException/getOperationException/

Done


> On Nov. 11, 2013, 7:52 p.m., Carl Steinbach wrote:
> > service/src/java/org/apache/hive/service/cli/operation/SQLOperation.java, line 71
> > <https://reviews.apache.org/r/15151/diff/3/?file=381919#file381919line71>
> >
> >     s/runException/operationException/

Done


> On Nov. 11, 2013, 7:52 p.m., Carl Steinbach wrote:
> > service/src/java/org/apache/hive/service/cli/operation/Operation.java, line 73
> > <https://reviews.apache.org/r/15151/diff/3/?file=381917#file381917line73>
> >
> >     Would it make sense to replace getState() and getRunException() with a getStatus() method that returns an object wrapping the operationState and operationException?
> >     
> >     If not, please change the name of getRunException to getException(), and add a comment explaining under what conditions this method will return a non-null value.

Done


> On Nov. 11, 2013, 7:52 p.m., Carl Steinbach wrote:
> > service/src/java/org/apache/hive/service/cli/operation/SQLOperation.java, line 324
> > <https://reviews.apache.org/r/15151/diff/3/?file=381919#file381919line324>
> >
> >     May as well just push this into Operation.

Done


> On Nov. 11, 2013, 7:52 p.m., Carl Steinbach wrote:
> > service/src/java/org/apache/hive/service/cli/thrift/ThriftCLIServiceClient.java, line 307
> > <https://reviews.apache.org/r/15151/diff/3/?file=381921#file381921line307>
> >
> >     Todo: set the status information.

Could you elaborate this a bit more?

Thanks for the feedback.


- Vaibhav


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


On Nov. 11, 2013, 7:23 p.m., Vaibhav Gumashta wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15151/
> -----------------------------------------------------------
> 
> (Updated Nov. 11, 2013, 7:23 p.m.)
> 
> 
> Review request for hive, Prasad Mujumdar and Thejas Nair.
> 
> 
> Bugs: HIVE-5230
>     https://issues.apache.org/jira/browse/HIVE-5230
> 
> 
> Repository: hive-git
> 
> 
> Description
> -------
> 
> [HIVE-4617|https://issues.apache.org/jira/browse/HIVE-4617] provides support for async execution in HS2. When a background thread gets an error, currently the client can only poll for the operation state and also the error with its stacktrace is logged. However, it will be useful to provide a richer error response like thrift API does with TStatus (which is constructed while building a Thrift response object). 
> 
> 
> Diffs
> -----
> 
>   itests/hive-unit/src/test/java/org/apache/hive/service/cli/CLIServiceTest.java PRE-CREATION 
>   itests/hive-unit/src/test/java/org/apache/hive/service/cli/thrift/ThriftCLIServiceTest.java PRE-CREATION 
>   jdbc/src/java/org/apache/hive/jdbc/HiveStatement.java fce19bf 
>   service/if/TCLIService.thrift 1f49445 
>   service/src/java/org/apache/hive/service/cli/CLIService.java 8c85386 
>   service/src/java/org/apache/hive/service/cli/CLIServiceClient.java 14ef54f 
>   service/src/java/org/apache/hive/service/cli/EmbeddedCLIServiceClient.java 9dca874 
>   service/src/java/org/apache/hive/service/cli/HiveSQLException.java 74e8b94 
>   service/src/java/org/apache/hive/service/cli/ICLIService.java f647ce6 
>   service/src/java/org/apache/hive/service/cli/OperationState.java 1ec6bd1 
>   service/src/java/org/apache/hive/service/cli/OperationStatus.java PRE-CREATION 
>   service/src/java/org/apache/hive/service/cli/operation/Operation.java 6f4b8dc 
>   service/src/java/org/apache/hive/service/cli/operation/OperationManager.java bcdb67f 
>   service/src/java/org/apache/hive/service/cli/operation/SQLOperation.java 4ee1b74 
>   service/src/java/org/apache/hive/service/cli/thrift/ThriftCLIService.java 9df110e 
>   service/src/java/org/apache/hive/service/cli/thrift/ThriftCLIServiceClient.java 9bb2a0f 
>   service/src/test/org/apache/hive/service/cli/CLIServiceTest.java cd9d99a 
>   service/src/test/org/apache/hive/service/cli/thrift/ThriftCLIServiceTest.java ff7166d 
> 
> Diff: https://reviews.apache.org/r/15151/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Vaibhav Gumashta
> 
>

Re: Review Request 15151: Better error reporting by async threads in HiveServer2

Posted by Vaibhav Gumashta <vg...@hortonworks.com>.


> On Nov. 11, 2013, 7:52 p.m., Carl Steinbach wrote:
> > service/src/java/org/apache/hive/service/cli/thrift/ThriftCLIServiceClient.java, line 307
> > <https://reviews.apache.org/r/15151/diff/3/?file=381921#file381921line307>
> >
> >     Todo: set the status information.
> 
> Vaibhav Gumashta wrote:
>     Could you elaborate this a bit more?
>     
>     Thanks for the feedback.

I think I got it.


- Vaibhav


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


On Nov. 12, 2013, 11:06 p.m., Vaibhav Gumashta wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15151/
> -----------------------------------------------------------
> 
> (Updated Nov. 12, 2013, 11:06 p.m.)
> 
> 
> Review request for hive, Carl Steinbach, Prasad Mujumdar, and Thejas Nair.
> 
> 
> Bugs: HIVE-5230
>     https://issues.apache.org/jira/browse/HIVE-5230
> 
> 
> Repository: hive-git
> 
> 
> Description
> -------
> 
> [HIVE-4617|https://issues.apache.org/jira/browse/HIVE-4617] provides support for async execution in HS2. When a background thread gets an error, currently the client can only poll for the operation state and also the error with its stacktrace is logged. However, it will be useful to provide a richer error response like thrift API does with TStatus (which is constructed while building a Thrift response object). 
> 
> 
> Diffs
> -----
> 
>   itests/hive-unit/src/test/java/org/apache/hive/service/cli/CLIServiceTest.java PRE-CREATION 
>   itests/hive-unit/src/test/java/org/apache/hive/service/cli/thrift/ThriftCLIServiceTest.java PRE-CREATION 
>   jdbc/src/java/org/apache/hive/jdbc/HiveStatement.java fce19bf 
>   service/if/TCLIService.thrift 1f49445 
>   service/src/java/org/apache/hive/service/cli/CLIService.java 8c85386 
>   service/src/java/org/apache/hive/service/cli/CLIServiceClient.java 14ef54f 
>   service/src/java/org/apache/hive/service/cli/EmbeddedCLIServiceClient.java 9dca874 
>   service/src/java/org/apache/hive/service/cli/HiveSQLException.java 74e8b94 
>   service/src/java/org/apache/hive/service/cli/ICLIService.java f647ce6 
>   service/src/java/org/apache/hive/service/cli/OperationState.java 1ec6bd1 
>   service/src/java/org/apache/hive/service/cli/OperationStatus.java PRE-CREATION 
>   service/src/java/org/apache/hive/service/cli/operation/Operation.java 6f4b8dc 
>   service/src/java/org/apache/hive/service/cli/operation/OperationManager.java bcdb67f 
>   service/src/java/org/apache/hive/service/cli/operation/SQLOperation.java 4ee1b74 
>   service/src/java/org/apache/hive/service/cli/thrift/ThriftCLIService.java 9df110e 
>   service/src/java/org/apache/hive/service/cli/thrift/ThriftCLIServiceClient.java 9bb2a0f 
>   service/src/test/org/apache/hive/service/cli/CLIServiceTest.java cd9d99a 
>   service/src/test/org/apache/hive/service/cli/thrift/ThriftCLIServiceTest.java ff7166d 
> 
> Diff: https://reviews.apache.org/r/15151/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Vaibhav Gumashta
> 
>

Re: Review Request 15151: Better error reporting by async threads in HiveServer2

Posted by Carl Steinbach <ca...@cloudera.com>.

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



itests/hive-unit/src/test/java/org/apache/hive/service/cli/CLIServiceTest.java
<https://reviews.apache.org/r/15151/#comment55613>

    Does this patch make any changes to CLIServiceTest or ThriftCLIServiceTest, or does it just move these files from service/ to itests/ ? If it does make changes can we move the files in a different patch?



service/if/TCLIService.thrift
<https://reviews.apache.org/r/15151/#comment55611>

    Need to add HIVE_CLI_SERVICE_PROTOCOL_V5 and update any references to HIVE_CLI_SERVICE_PROTOCOL_V4.



service/if/TCLIService.thrift
<https://reviews.apache.org/r/15151/#comment55615>

    Please reuse TStatus instead of adding a new struct.



service/if/TCLIService.thrift
<https://reviews.apache.org/r/15151/#comment55610>

    This modification will break compatibility between upversion/downversion clients and servers since it modifies the type of an existing field. 
    
    It's possible to avoid this problem by instead adding a new "TStatus operationStatus" field.



service/src/java/org/apache/hive/service/cli/CLIService.java
<https://reviews.apache.org/r/15151/#comment55619>

    Please push this logic into OperationManager.getOperationStatus()



service/src/java/org/apache/hive/service/cli/operation/Operation.java
<https://reviews.apache.org/r/15151/#comment55621>

    Would it make sense to replace getState() and getRunException() with a getStatus() method that returns an object wrapping the operationState and operationException?
    
    If not, please change the name of getRunException to getException(), and add a comment explaining under what conditions this method will return a non-null value.



service/src/java/org/apache/hive/service/cli/operation/OperationManager.java
<https://reviews.apache.org/r/15151/#comment55625>

    s/getOperationRunException/getOperationException/



service/src/java/org/apache/hive/service/cli/operation/SQLOperation.java
<https://reviews.apache.org/r/15151/#comment55626>

    s/runException/operationException/



service/src/java/org/apache/hive/service/cli/operation/SQLOperation.java
<https://reviews.apache.org/r/15151/#comment55633>

    May as well just push this into Operation.



service/src/java/org/apache/hive/service/cli/thrift/ThriftCLIServiceClient.java
<https://reviews.apache.org/r/15151/#comment55631>

    Todo: set the status information.


- Carl Steinbach


On Nov. 11, 2013, 7:23 p.m., Vaibhav Gumashta wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15151/
> -----------------------------------------------------------
> 
> (Updated Nov. 11, 2013, 7:23 p.m.)
> 
> 
> Review request for hive, Prasad Mujumdar and Thejas Nair.
> 
> 
> Bugs: HIVE-5230
>     https://issues.apache.org/jira/browse/HIVE-5230
> 
> 
> Repository: hive-git
> 
> 
> Description
> -------
> 
> [HIVE-4617|https://issues.apache.org/jira/browse/HIVE-4617] provides support for async execution in HS2. When a background thread gets an error, currently the client can only poll for the operation state and also the error with its stacktrace is logged. However, it will be useful to provide a richer error response like thrift API does with TStatus (which is constructed while building a Thrift response object). 
> 
> 
> Diffs
> -----
> 
>   itests/hive-unit/src/test/java/org/apache/hive/service/cli/CLIServiceTest.java PRE-CREATION 
>   itests/hive-unit/src/test/java/org/apache/hive/service/cli/thrift/ThriftCLIServiceTest.java PRE-CREATION 
>   jdbc/src/java/org/apache/hive/jdbc/HiveStatement.java fce19bf 
>   service/if/TCLIService.thrift 1f49445 
>   service/src/java/org/apache/hive/service/cli/CLIService.java 8c85386 
>   service/src/java/org/apache/hive/service/cli/CLIServiceClient.java 14ef54f 
>   service/src/java/org/apache/hive/service/cli/EmbeddedCLIServiceClient.java 9dca874 
>   service/src/java/org/apache/hive/service/cli/HiveSQLException.java 74e8b94 
>   service/src/java/org/apache/hive/service/cli/ICLIService.java f647ce6 
>   service/src/java/org/apache/hive/service/cli/OperationState.java 1ec6bd1 
>   service/src/java/org/apache/hive/service/cli/OperationStatus.java PRE-CREATION 
>   service/src/java/org/apache/hive/service/cli/operation/Operation.java 6f4b8dc 
>   service/src/java/org/apache/hive/service/cli/operation/OperationManager.java bcdb67f 
>   service/src/java/org/apache/hive/service/cli/operation/SQLOperation.java 4ee1b74 
>   service/src/java/org/apache/hive/service/cli/thrift/ThriftCLIService.java 9df110e 
>   service/src/java/org/apache/hive/service/cli/thrift/ThriftCLIServiceClient.java 9bb2a0f 
>   service/src/test/org/apache/hive/service/cli/CLIServiceTest.java cd9d99a 
>   service/src/test/org/apache/hive/service/cli/thrift/ThriftCLIServiceTest.java ff7166d 
> 
> Diff: https://reviews.apache.org/r/15151/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Vaibhav Gumashta
> 
>

Re: Review Request 15151: Better error reporting by async threads in HiveServer2

Posted by Vaibhav Gumashta <vg...@hortonworks.com>.

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

(Updated Dec. 17, 2013, 5:34 a.m.)


Review request for hive, Carl Steinbach, Prasad Mujumdar, and Thejas Nair.


Changes
-------

Rebased on trunk with thrift files.


Bugs: HIVE-5230
    https://issues.apache.org/jira/browse/HIVE-5230


Repository: hive-git


Description
-------

[HIVE-4617|https://issues.apache.org/jira/browse/HIVE-4617] provides support for async execution in HS2. When a background thread gets an error, currently the client can only poll for the operation state and also the error with its stacktrace is logged. However, it will be useful to provide a richer error response like thrift API does with TStatus (which is constructed while building a Thrift response object). 


Diffs (updated)
-----

  jdbc/src/java/org/apache/hive/jdbc/HiveConnection.java e420b75 
  jdbc/src/java/org/apache/hive/jdbc/HiveStatement.java 37975e5 
  service/if/TCLIService.thrift 62a9730 
  service/src/gen/thrift/gen-cpp/TCLIService_types.h 853bb4c 
  service/src/gen/thrift/gen-cpp/TCLIService_types.cpp 7ab1310 
  service/src/gen/thrift/gen-javabean/org/apache/hadoop/hive/service/ThriftHive.java 1c44789 
  service/src/gen/thrift/gen-javabean/org/apache/hive/service/cli/thrift/TColumn.java 497cc01 
  service/src/gen/thrift/gen-javabean/org/apache/hive/service/cli/thrift/TGetOperationStatusResp.java b5c3f01 
  service/src/gen/thrift/gen-javabean/org/apache/hive/service/cli/thrift/TGetTablesReq.java 1cb5147 
  service/src/gen/thrift/gen-javabean/org/apache/hive/service/cli/thrift/TOpenSessionReq.java 8ab8297 
  service/src/gen/thrift/gen-javabean/org/apache/hive/service/cli/thrift/TOpenSessionResp.java 688f790 
  service/src/gen/thrift/gen-javabean/org/apache/hive/service/cli/thrift/TProtocolVersion.java 8c6c4f0 
  service/src/gen/thrift/gen-javabean/org/apache/hive/service/cli/thrift/TRow.java 0b6772c 
  service/src/gen/thrift/gen-javabean/org/apache/hive/service/cli/thrift/TRowSet.java db2262d 
  service/src/gen/thrift/gen-javabean/org/apache/hive/service/cli/thrift/TStatus.java 81c2f16 
  service/src/gen/thrift/gen-javabean/org/apache/hive/service/cli/thrift/TTableSchema.java ff5e54d 
  service/src/gen/thrift/gen-javabean/org/apache/hive/service/cli/thrift/TTypeDesc.java 251f86a 
  service/src/gen/thrift/gen-py/TCLIService/ttypes.py 185ea5b 
  service/src/gen/thrift/gen-rb/t_c_l_i_service_types.rb c94acbf 
  service/src/java/org/apache/hive/service/cli/CLIService.java 8c85386 
  service/src/java/org/apache/hive/service/cli/CLIServiceClient.java 14ef54f 
  service/src/java/org/apache/hive/service/cli/EmbeddedCLIServiceClient.java 9dca874 
  service/src/java/org/apache/hive/service/cli/ICLIService.java f647ce6 
  service/src/java/org/apache/hive/service/cli/OperationState.java 1ec6bd1 
  service/src/java/org/apache/hive/service/cli/OperationStatus.java PRE-CREATION 
  service/src/java/org/apache/hive/service/cli/operation/Operation.java 5d1dd5f 
  service/src/java/org/apache/hive/service/cli/operation/OperationManager.java bcdb67f 
  service/src/java/org/apache/hive/service/cli/operation/SQLOperation.java 296f8b3 
  service/src/java/org/apache/hive/service/cli/session/HiveSessionImpl.java c0e6151 
  service/src/java/org/apache/hive/service/cli/session/SessionManager.java e262b72 
  service/src/java/org/apache/hive/service/cli/thrift/ThriftCLIService.java 9df110e 
  service/src/java/org/apache/hive/service/cli/thrift/ThriftCLIServiceClient.java 9bb2a0f 
  service/src/test/org/apache/hive/service/cli/CLIServiceTest.java 44d3130 
  service/src/test/org/apache/hive/service/cli/thrift/ThriftCLIServiceTest.java ff7166d 

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


Testing
-------


Thanks,

Vaibhav Gumashta

Re: Review Request 15151: Better error reporting by async threads in HiveServer2

Posted by Vaibhav Gumashta <vg...@hortonworks.com>.

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

(Updated Dec. 5, 2013, 12:16 a.m.)


Review request for hive, Carl Steinbach, Prasad Mujumdar, and Thejas Nair.


Changes
-------

Fixed formatting issues.


Bugs: HIVE-5230
    https://issues.apache.org/jira/browse/HIVE-5230


Repository: hive-git


Description
-------

[HIVE-4617|https://issues.apache.org/jira/browse/HIVE-4617] provides support for async execution in HS2. When a background thread gets an error, currently the client can only poll for the operation state and also the error with its stacktrace is logged. However, it will be useful to provide a richer error response like thrift API does with TStatus (which is constructed while building a Thrift response object). 


Diffs (updated)
-----

  jdbc/src/java/org/apache/hive/jdbc/HiveConnection.java ef39573 
  jdbc/src/java/org/apache/hive/jdbc/HiveStatement.java fce19bf 
  service/if/TCLIService.thrift 1f49445 
  service/src/java/org/apache/hive/service/cli/CLIService.java 8c85386 
  service/src/java/org/apache/hive/service/cli/CLIServiceClient.java 14ef54f 
  service/src/java/org/apache/hive/service/cli/EmbeddedCLIServiceClient.java 9dca874 
  service/src/java/org/apache/hive/service/cli/HiveSQLException.java 74e8b94 
  service/src/java/org/apache/hive/service/cli/ICLIService.java f647ce6 
  service/src/java/org/apache/hive/service/cli/OperationState.java 1ec6bd1 
  service/src/java/org/apache/hive/service/cli/OperationStatus.java PRE-CREATION 
  service/src/java/org/apache/hive/service/cli/operation/Operation.java 6f4b8dc 
  service/src/java/org/apache/hive/service/cli/operation/OperationManager.java bcdb67f 
  service/src/java/org/apache/hive/service/cli/operation/SQLOperation.java 4ee1b74 
  service/src/java/org/apache/hive/service/cli/session/SessionManager.java f392d62 
  service/src/java/org/apache/hive/service/cli/thrift/ThriftCLIService.java 9df110e 
  service/src/java/org/apache/hive/service/cli/thrift/ThriftCLIServiceClient.java 9bb2a0f 
  service/src/test/org/apache/hive/service/cli/CLIServiceTest.java cd9d99a 
  service/src/test/org/apache/hive/service/cli/thrift/ThriftCLIServiceTest.java ff7166d 

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


Testing
-------


Thanks,

Vaibhav Gumashta

Re: Review Request 15151: Better error reporting by async threads in HiveServer2

Posted by Vaibhav Gumashta <vg...@hortonworks.com>.

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

(Updated Nov. 15, 2013, 8:02 p.m.)


Review request for hive, Carl Steinbach, Prasad Mujumdar, and Thejas Nair.


Bugs: HIVE-5230
    https://issues.apache.org/jira/browse/HIVE-5230


Repository: hive-git


Description
-------

[HIVE-4617|https://issues.apache.org/jira/browse/HIVE-4617] provides support for async execution in HS2. When a background thread gets an error, currently the client can only poll for the operation state and also the error with its stacktrace is logged. However, it will be useful to provide a richer error response like thrift API does with TStatus (which is constructed while building a Thrift response object). 


Diffs (updated)
-----

  jdbc/src/java/org/apache/hive/jdbc/HiveConnection.java ef39573 
  jdbc/src/java/org/apache/hive/jdbc/HiveStatement.java fce19bf 
  service/if/TCLIService.thrift 1f49445 
  service/src/java/org/apache/hive/service/cli/CLIService.java 8c85386 
  service/src/java/org/apache/hive/service/cli/CLIServiceClient.java 14ef54f 
  service/src/java/org/apache/hive/service/cli/EmbeddedCLIServiceClient.java 9dca874 
  service/src/java/org/apache/hive/service/cli/HiveSQLException.java 74e8b94 
  service/src/java/org/apache/hive/service/cli/ICLIService.java f647ce6 
  service/src/java/org/apache/hive/service/cli/OperationState.java 1ec6bd1 
  service/src/java/org/apache/hive/service/cli/OperationStatus.java PRE-CREATION 
  service/src/java/org/apache/hive/service/cli/operation/Operation.java 6f4b8dc 
  service/src/java/org/apache/hive/service/cli/operation/OperationManager.java bcdb67f 
  service/src/java/org/apache/hive/service/cli/operation/SQLOperation.java 4ee1b74 
  service/src/java/org/apache/hive/service/cli/session/SessionManager.java f392d62 
  service/src/java/org/apache/hive/service/cli/thrift/ThriftCLIService.java 9df110e 
  service/src/java/org/apache/hive/service/cli/thrift/ThriftCLIServiceClient.java 9bb2a0f 
  service/src/test/org/apache/hive/service/cli/CLIServiceTest.java cd9d99a 
  service/src/test/org/apache/hive/service/cli/thrift/ThriftCLIServiceTest.java ff7166d 

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


Testing
-------


Thanks,

Vaibhav Gumashta

Re: Review Request 15151: Better error reporting by async threads in HiveServer2

Posted by Vaibhav Gumashta <vg...@hortonworks.com>.

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

(Updated Nov. 15, 2013, 8 p.m.)


Review request for hive, Carl Steinbach, Prasad Mujumdar, and Thejas Nair.


Bugs: HIVE-5230
    https://issues.apache.org/jira/browse/HIVE-5230


Repository: hive-git


Description
-------

[HIVE-4617|https://issues.apache.org/jira/browse/HIVE-4617] provides support for async execution in HS2. When a background thread gets an error, currently the client can only poll for the operation state and also the error with its stacktrace is logged. However, it will be useful to provide a richer error response like thrift API does with TStatus (which is constructed while building a Thrift response object). 


Diffs
-----

  jdbc/src/java/org/apache/hive/jdbc/HiveConnection.java ef39573 
  jdbc/src/java/org/apache/hive/jdbc/HiveStatement.java fce19bf 
  service/if/TCLIService.thrift 1f49445 
  service/src/java/org/apache/hive/service/cli/CLIService.java 8c85386 
  service/src/java/org/apache/hive/service/cli/CLIServiceClient.java 14ef54f 
  service/src/java/org/apache/hive/service/cli/EmbeddedCLIServiceClient.java 9dca874 
  service/src/java/org/apache/hive/service/cli/HiveSQLException.java 74e8b94 
  service/src/java/org/apache/hive/service/cli/ICLIService.java f647ce6 
  service/src/java/org/apache/hive/service/cli/OperationState.java 1ec6bd1 
  service/src/java/org/apache/hive/service/cli/OperationStatus.java PRE-CREATION 
  service/src/java/org/apache/hive/service/cli/operation/Operation.java 6f4b8dc 
  service/src/java/org/apache/hive/service/cli/operation/OperationManager.java bcdb67f 
  service/src/java/org/apache/hive/service/cli/operation/SQLOperation.java 4ee1b74 
  service/src/java/org/apache/hive/service/cli/session/SessionManager.java f392d62 
  service/src/java/org/apache/hive/service/cli/thrift/ThriftCLIService.java 9df110e 
  service/src/java/org/apache/hive/service/cli/thrift/ThriftCLIServiceClient.java 9bb2a0f 
  service/src/test/org/apache/hive/service/cli/CLIServiceTest.java cd9d99a 
  service/src/test/org/apache/hive/service/cli/thrift/ThriftCLIServiceTest.java ff7166d 

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


Testing
-------


Thanks,

Vaibhav Gumashta

Re: Review Request 15151: Better error reporting by async threads in HiveServer2

Posted by Vaibhav Gumashta <vg...@hortonworks.com>.

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

(Updated Nov. 15, 2013, 7:59 p.m.)


Review request for hive, Carl Steinbach, Prasad Mujumdar, and Thejas Nair.


Bugs: HIVE-5230
    https://issues.apache.org/jira/browse/HIVE-5230


Repository: hive-git


Description
-------

[HIVE-4617|https://issues.apache.org/jira/browse/HIVE-4617] provides support for async execution in HS2. When a background thread gets an error, currently the client can only poll for the operation state and also the error with its stacktrace is logged. However, it will be useful to provide a richer error response like thrift API does with TStatus (which is constructed while building a Thrift response object). 


Diffs
-----

  jdbc/src/java/org/apache/hive/jdbc/HiveConnection.java ef39573 
  jdbc/src/java/org/apache/hive/jdbc/HiveStatement.java fce19bf 
  service/if/TCLIService.thrift 1f49445 
  service/src/java/org/apache/hive/service/cli/CLIService.java 8c85386 
  service/src/java/org/apache/hive/service/cli/CLIServiceClient.java 14ef54f 
  service/src/java/org/apache/hive/service/cli/EmbeddedCLIServiceClient.java 9dca874 
  service/src/java/org/apache/hive/service/cli/HiveSQLException.java 74e8b94 
  service/src/java/org/apache/hive/service/cli/ICLIService.java f647ce6 
  service/src/java/org/apache/hive/service/cli/OperationState.java 1ec6bd1 
  service/src/java/org/apache/hive/service/cli/OperationStatus.java PRE-CREATION 
  service/src/java/org/apache/hive/service/cli/operation/Operation.java 6f4b8dc 
  service/src/java/org/apache/hive/service/cli/operation/OperationManager.java bcdb67f 
  service/src/java/org/apache/hive/service/cli/operation/SQLOperation.java 4ee1b74 
  service/src/java/org/apache/hive/service/cli/session/SessionManager.java f392d62 
  service/src/java/org/apache/hive/service/cli/thrift/ThriftCLIService.java 9df110e 
  service/src/java/org/apache/hive/service/cli/thrift/ThriftCLIServiceClient.java 9bb2a0f 
  service/src/test/org/apache/hive/service/cli/CLIServiceTest.java cd9d99a 
  service/src/test/org/apache/hive/service/cli/thrift/ThriftCLIServiceTest.java ff7166d 

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


Testing
-------


Thanks,

Vaibhav Gumashta

Re: Review Request 15151: Better error reporting by async threads in HiveServer2

Posted by Vaibhav Gumashta <vg...@hortonworks.com>.

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

(Updated Nov. 15, 2013, 7:59 p.m.)


Review request for hive, Carl Steinbach, Prasad Mujumdar, and Thejas Nair.


Changes
-------

Latest feedback.


Bugs: HIVE-5230
    https://issues.apache.org/jira/browse/HIVE-5230


Repository: hive-git


Description
-------

[HIVE-4617|https://issues.apache.org/jira/browse/HIVE-4617] provides support for async execution in HS2. When a background thread gets an error, currently the client can only poll for the operation state and also the error with its stacktrace is logged. However, it will be useful to provide a richer error response like thrift API does with TStatus (which is constructed while building a Thrift response object). 


Diffs (updated)
-----

  jdbc/src/java/org/apache/hive/jdbc/HiveConnection.java ef39573 
  jdbc/src/java/org/apache/hive/jdbc/HiveStatement.java fce19bf 
  service/if/TCLIService.thrift 1f49445 
  service/src/java/org/apache/hive/service/cli/CLIService.java 8c85386 
  service/src/java/org/apache/hive/service/cli/CLIServiceClient.java 14ef54f 
  service/src/java/org/apache/hive/service/cli/EmbeddedCLIServiceClient.java 9dca874 
  service/src/java/org/apache/hive/service/cli/HiveSQLException.java 74e8b94 
  service/src/java/org/apache/hive/service/cli/ICLIService.java f647ce6 
  service/src/java/org/apache/hive/service/cli/OperationState.java 1ec6bd1 
  service/src/java/org/apache/hive/service/cli/OperationStatus.java PRE-CREATION 
  service/src/java/org/apache/hive/service/cli/operation/Operation.java 6f4b8dc 
  service/src/java/org/apache/hive/service/cli/operation/OperationManager.java bcdb67f 
  service/src/java/org/apache/hive/service/cli/operation/SQLOperation.java 4ee1b74 
  service/src/java/org/apache/hive/service/cli/session/SessionManager.java f392d62 
  service/src/java/org/apache/hive/service/cli/thrift/ThriftCLIService.java 9df110e 
  service/src/java/org/apache/hive/service/cli/thrift/ThriftCLIServiceClient.java 9bb2a0f 
  service/src/test/org/apache/hive/service/cli/CLIServiceTest.java cd9d99a 
  service/src/test/org/apache/hive/service/cli/thrift/ThriftCLIServiceTest.java ff7166d 

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


Testing
-------


Thanks,

Vaibhav Gumashta

Re: Review Request 15151: Better error reporting by async threads in HiveServer2

Posted by Vaibhav Gumashta <vg...@hortonworks.com>.


> On Nov. 15, 2013, 9:02 a.m., Carl Steinbach wrote:
> > service/if/TCLIService.thrift, line 491
> > <https://reviews.apache.org/r/15151/diff/5/?file=383817#file383817line491>
> >
> >     Please bump this to V5. Also, does the V5 client now make any assumptions about the server being V5 too? Do we need to add any conditional version detection logic to the JDBC driver or CLIServiceClient?
> >     
> >     Do we currently log the client version on the server side and vice versa? If not it would probably be good to add this.

Ya, we do check the version match in JDBC but not in ThriftCLIServiceClient. My bad, I didn't run the JDBC test after the patch that bumped the version to V5. Added the client version logging on the server side.


- Vaibhav


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


On Nov. 13, 2013, 9:54 p.m., Vaibhav Gumashta wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15151/
> -----------------------------------------------------------
> 
> (Updated Nov. 13, 2013, 9:54 p.m.)
> 
> 
> Review request for hive, Carl Steinbach, Prasad Mujumdar, and Thejas Nair.
> 
> 
> Bugs: HIVE-5230
>     https://issues.apache.org/jira/browse/HIVE-5230
> 
> 
> Repository: hive-git
> 
> 
> Description
> -------
> 
> [HIVE-4617|https://issues.apache.org/jira/browse/HIVE-4617] provides support for async execution in HS2. When a background thread gets an error, currently the client can only poll for the operation state and also the error with its stacktrace is logged. However, it will be useful to provide a richer error response like thrift API does with TStatus (which is constructed while building a Thrift response object). 
> 
> 
> Diffs
> -----
> 
>   jdbc/src/java/org/apache/hive/jdbc/HiveConnection.java ef39573 
>   jdbc/src/java/org/apache/hive/jdbc/HiveStatement.java fce19bf 
>   service/if/TCLIService.thrift 1f49445 
>   service/src/java/org/apache/hive/service/cli/CLIService.java 8c85386 
>   service/src/java/org/apache/hive/service/cli/CLIServiceClient.java 14ef54f 
>   service/src/java/org/apache/hive/service/cli/EmbeddedCLIServiceClient.java 9dca874 
>   service/src/java/org/apache/hive/service/cli/HiveSQLException.java 74e8b94 
>   service/src/java/org/apache/hive/service/cli/ICLIService.java f647ce6 
>   service/src/java/org/apache/hive/service/cli/OperationState.java 1ec6bd1 
>   service/src/java/org/apache/hive/service/cli/OperationStatus.java PRE-CREATION 
>   service/src/java/org/apache/hive/service/cli/operation/Operation.java 6f4b8dc 
>   service/src/java/org/apache/hive/service/cli/operation/OperationManager.java bcdb67f 
>   service/src/java/org/apache/hive/service/cli/operation/SQLOperation.java 4ee1b74 
>   service/src/java/org/apache/hive/service/cli/session/SessionManager.java f392d62 
>   service/src/java/org/apache/hive/service/cli/thrift/ThriftCLIService.java 9df110e 
>   service/src/java/org/apache/hive/service/cli/thrift/ThriftCLIServiceClient.java 9bb2a0f 
>   service/src/test/org/apache/hive/service/cli/CLIServiceTest.java cd9d99a 
>   service/src/test/org/apache/hive/service/cli/thrift/ThriftCLIServiceTest.java ff7166d 
> 
> Diff: https://reviews.apache.org/r/15151/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Vaibhav Gumashta
> 
>

Re: Review Request 15151: Better error reporting by async threads in HiveServer2

Posted by Carl Steinbach <cw...@apache.org>.

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



service/if/TCLIService.thrift
<https://reviews.apache.org/r/15151/#comment56004>

    Please bump this to V5. Also, does the V5 client now make any assumptions about the server being V5 too? Do we need to add any conditional version detection logic to the JDBC driver or CLIServiceClient?
    
    Do we currently log the client version on the server side and vice versa? If not it would probably be good to add this.



service/if/TCLIService.thrift
<https://reviews.apache.org/r/15151/#comment56005>

    Please bump this to V5.


- Carl Steinbach


On Nov. 13, 2013, 9:54 p.m., Vaibhav Gumashta wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15151/
> -----------------------------------------------------------
> 
> (Updated Nov. 13, 2013, 9:54 p.m.)
> 
> 
> Review request for hive, Carl Steinbach, Prasad Mujumdar, and Thejas Nair.
> 
> 
> Bugs: HIVE-5230
>     https://issues.apache.org/jira/browse/HIVE-5230
> 
> 
> Repository: hive-git
> 
> 
> Description
> -------
> 
> [HIVE-4617|https://issues.apache.org/jira/browse/HIVE-4617] provides support for async execution in HS2. When a background thread gets an error, currently the client can only poll for the operation state and also the error with its stacktrace is logged. However, it will be useful to provide a richer error response like thrift API does with TStatus (which is constructed while building a Thrift response object). 
> 
> 
> Diffs
> -----
> 
>   jdbc/src/java/org/apache/hive/jdbc/HiveConnection.java ef39573 
>   jdbc/src/java/org/apache/hive/jdbc/HiveStatement.java fce19bf 
>   service/if/TCLIService.thrift 1f49445 
>   service/src/java/org/apache/hive/service/cli/CLIService.java 8c85386 
>   service/src/java/org/apache/hive/service/cli/CLIServiceClient.java 14ef54f 
>   service/src/java/org/apache/hive/service/cli/EmbeddedCLIServiceClient.java 9dca874 
>   service/src/java/org/apache/hive/service/cli/HiveSQLException.java 74e8b94 
>   service/src/java/org/apache/hive/service/cli/ICLIService.java f647ce6 
>   service/src/java/org/apache/hive/service/cli/OperationState.java 1ec6bd1 
>   service/src/java/org/apache/hive/service/cli/OperationStatus.java PRE-CREATION 
>   service/src/java/org/apache/hive/service/cli/operation/Operation.java 6f4b8dc 
>   service/src/java/org/apache/hive/service/cli/operation/OperationManager.java bcdb67f 
>   service/src/java/org/apache/hive/service/cli/operation/SQLOperation.java 4ee1b74 
>   service/src/java/org/apache/hive/service/cli/session/SessionManager.java f392d62 
>   service/src/java/org/apache/hive/service/cli/thrift/ThriftCLIService.java 9df110e 
>   service/src/java/org/apache/hive/service/cli/thrift/ThriftCLIServiceClient.java 9bb2a0f 
>   service/src/test/org/apache/hive/service/cli/CLIServiceTest.java cd9d99a 
>   service/src/test/org/apache/hive/service/cli/thrift/ThriftCLIServiceTest.java ff7166d 
> 
> Diff: https://reviews.apache.org/r/15151/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Vaibhav Gumashta
> 
>

Re: Review Request 15151: Better error reporting by async threads in HiveServer2

Posted by Carl Steinbach <cw...@apache.org>.

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



service/if/TCLIService.thrift
<https://reviews.apache.org/r/15151/#comment56003>

    Ok, good point. Please disregard.


- Carl Steinbach


On Nov. 13, 2013, 9:54 p.m., Vaibhav Gumashta wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15151/
> -----------------------------------------------------------
> 
> (Updated Nov. 13, 2013, 9:54 p.m.)
> 
> 
> Review request for hive, Carl Steinbach, Prasad Mujumdar, and Thejas Nair.
> 
> 
> Bugs: HIVE-5230
>     https://issues.apache.org/jira/browse/HIVE-5230
> 
> 
> Repository: hive-git
> 
> 
> Description
> -------
> 
> [HIVE-4617|https://issues.apache.org/jira/browse/HIVE-4617] provides support for async execution in HS2. When a background thread gets an error, currently the client can only poll for the operation state and also the error with its stacktrace is logged. However, it will be useful to provide a richer error response like thrift API does with TStatus (which is constructed while building a Thrift response object). 
> 
> 
> Diffs
> -----
> 
>   jdbc/src/java/org/apache/hive/jdbc/HiveConnection.java ef39573 
>   jdbc/src/java/org/apache/hive/jdbc/HiveStatement.java fce19bf 
>   service/if/TCLIService.thrift 1f49445 
>   service/src/java/org/apache/hive/service/cli/CLIService.java 8c85386 
>   service/src/java/org/apache/hive/service/cli/CLIServiceClient.java 14ef54f 
>   service/src/java/org/apache/hive/service/cli/EmbeddedCLIServiceClient.java 9dca874 
>   service/src/java/org/apache/hive/service/cli/HiveSQLException.java 74e8b94 
>   service/src/java/org/apache/hive/service/cli/ICLIService.java f647ce6 
>   service/src/java/org/apache/hive/service/cli/OperationState.java 1ec6bd1 
>   service/src/java/org/apache/hive/service/cli/OperationStatus.java PRE-CREATION 
>   service/src/java/org/apache/hive/service/cli/operation/Operation.java 6f4b8dc 
>   service/src/java/org/apache/hive/service/cli/operation/OperationManager.java bcdb67f 
>   service/src/java/org/apache/hive/service/cli/operation/SQLOperation.java 4ee1b74 
>   service/src/java/org/apache/hive/service/cli/session/SessionManager.java f392d62 
>   service/src/java/org/apache/hive/service/cli/thrift/ThriftCLIService.java 9df110e 
>   service/src/java/org/apache/hive/service/cli/thrift/ThriftCLIServiceClient.java 9bb2a0f 
>   service/src/test/org/apache/hive/service/cli/CLIServiceTest.java cd9d99a 
>   service/src/test/org/apache/hive/service/cli/thrift/ThriftCLIServiceTest.java ff7166d 
> 
> Diff: https://reviews.apache.org/r/15151/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Vaibhav Gumashta
> 
>

Re: Review Request 15151: Better error reporting by async threads in HiveServer2

Posted by Vaibhav Gumashta <vg...@hortonworks.com>.

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

(Updated Nov. 13, 2013, 9:54 p.m.)


Review request for hive, Carl Steinbach, Prasad Mujumdar, and Thejas Nair.


Bugs: HIVE-5230
    https://issues.apache.org/jira/browse/HIVE-5230


Repository: hive-git


Description
-------

[HIVE-4617|https://issues.apache.org/jira/browse/HIVE-4617] provides support for async execution in HS2. When a background thread gets an error, currently the client can only poll for the operation state and also the error with its stacktrace is logged. However, it will be useful to provide a richer error response like thrift API does with TStatus (which is constructed while building a Thrift response object). 


Diffs
-----

  jdbc/src/java/org/apache/hive/jdbc/HiveConnection.java ef39573 
  jdbc/src/java/org/apache/hive/jdbc/HiveStatement.java fce19bf 
  service/if/TCLIService.thrift 1f49445 
  service/src/java/org/apache/hive/service/cli/CLIService.java 8c85386 
  service/src/java/org/apache/hive/service/cli/CLIServiceClient.java 14ef54f 
  service/src/java/org/apache/hive/service/cli/EmbeddedCLIServiceClient.java 9dca874 
  service/src/java/org/apache/hive/service/cli/HiveSQLException.java 74e8b94 
  service/src/java/org/apache/hive/service/cli/ICLIService.java f647ce6 
  service/src/java/org/apache/hive/service/cli/OperationState.java 1ec6bd1 
  service/src/java/org/apache/hive/service/cli/OperationStatus.java PRE-CREATION 
  service/src/java/org/apache/hive/service/cli/operation/Operation.java 6f4b8dc 
  service/src/java/org/apache/hive/service/cli/operation/OperationManager.java bcdb67f 
  service/src/java/org/apache/hive/service/cli/operation/SQLOperation.java 4ee1b74 
  service/src/java/org/apache/hive/service/cli/session/SessionManager.java f392d62 
  service/src/java/org/apache/hive/service/cli/thrift/ThriftCLIService.java 9df110e 
  service/src/java/org/apache/hive/service/cli/thrift/ThriftCLIServiceClient.java 9bb2a0f 
  service/src/test/org/apache/hive/service/cli/CLIServiceTest.java cd9d99a 
  service/src/test/org/apache/hive/service/cli/thrift/ThriftCLIServiceTest.java ff7166d 

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


Testing
-------


Thanks,

Vaibhav Gumashta

Re: Review Request 15151: Better error reporting by async threads in HiveServer2

Posted by Vaibhav Gumashta <vg...@hortonworks.com>.

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

(Updated Nov. 13, 2013, 9:54 p.m.)


Review request for hive, Carl Steinbach, Prasad Mujumdar, and Thejas Nair.


Changes
-------

Feedback incorporated.


Bugs: HIVE-5230
    https://issues.apache.org/jira/browse/HIVE-5230


Repository: hive-git


Description
-------

[HIVE-4617|https://issues.apache.org/jira/browse/HIVE-4617] provides support for async execution in HS2. When a background thread gets an error, currently the client can only poll for the operation state and also the error with its stacktrace is logged. However, it will be useful to provide a richer error response like thrift API does with TStatus (which is constructed while building a Thrift response object). 


Diffs (updated)
-----

  jdbc/src/java/org/apache/hive/jdbc/HiveConnection.java ef39573 
  jdbc/src/java/org/apache/hive/jdbc/HiveStatement.java fce19bf 
  service/if/TCLIService.thrift 1f49445 
  service/src/java/org/apache/hive/service/cli/CLIService.java 8c85386 
  service/src/java/org/apache/hive/service/cli/CLIServiceClient.java 14ef54f 
  service/src/java/org/apache/hive/service/cli/EmbeddedCLIServiceClient.java 9dca874 
  service/src/java/org/apache/hive/service/cli/HiveSQLException.java 74e8b94 
  service/src/java/org/apache/hive/service/cli/ICLIService.java f647ce6 
  service/src/java/org/apache/hive/service/cli/OperationState.java 1ec6bd1 
  service/src/java/org/apache/hive/service/cli/OperationStatus.java PRE-CREATION 
  service/src/java/org/apache/hive/service/cli/operation/Operation.java 6f4b8dc 
  service/src/java/org/apache/hive/service/cli/operation/OperationManager.java bcdb67f 
  service/src/java/org/apache/hive/service/cli/operation/SQLOperation.java 4ee1b74 
  service/src/java/org/apache/hive/service/cli/session/SessionManager.java f392d62 
  service/src/java/org/apache/hive/service/cli/thrift/ThriftCLIService.java 9df110e 
  service/src/java/org/apache/hive/service/cli/thrift/ThriftCLIServiceClient.java 9bb2a0f 
  service/src/test/org/apache/hive/service/cli/CLIServiceTest.java cd9d99a 
  service/src/test/org/apache/hive/service/cli/thrift/ThriftCLIServiceTest.java ff7166d 

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


Testing
-------


Thanks,

Vaibhav Gumashta

Re: Review Request 15151: Better error reporting by async threads in HiveServer2

Posted by Vaibhav Gumashta <vg...@hortonworks.com>.

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

(Updated Nov. 12, 2013, 11:06 p.m.)


Review request for hive, Carl Steinbach, Prasad Mujumdar, and Thejas Nair.


Bugs: HIVE-5230
    https://issues.apache.org/jira/browse/HIVE-5230


Repository: hive-git


Description
-------

[HIVE-4617|https://issues.apache.org/jira/browse/HIVE-4617] provides support for async execution in HS2. When a background thread gets an error, currently the client can only poll for the operation state and also the error with its stacktrace is logged. However, it will be useful to provide a richer error response like thrift API does with TStatus (which is constructed while building a Thrift response object). 


Diffs
-----

  itests/hive-unit/src/test/java/org/apache/hive/service/cli/CLIServiceTest.java PRE-CREATION 
  itests/hive-unit/src/test/java/org/apache/hive/service/cli/thrift/ThriftCLIServiceTest.java PRE-CREATION 
  jdbc/src/java/org/apache/hive/jdbc/HiveStatement.java fce19bf 
  service/if/TCLIService.thrift 1f49445 
  service/src/java/org/apache/hive/service/cli/CLIService.java 8c85386 
  service/src/java/org/apache/hive/service/cli/CLIServiceClient.java 14ef54f 
  service/src/java/org/apache/hive/service/cli/EmbeddedCLIServiceClient.java 9dca874 
  service/src/java/org/apache/hive/service/cli/HiveSQLException.java 74e8b94 
  service/src/java/org/apache/hive/service/cli/ICLIService.java f647ce6 
  service/src/java/org/apache/hive/service/cli/OperationState.java 1ec6bd1 
  service/src/java/org/apache/hive/service/cli/OperationStatus.java PRE-CREATION 
  service/src/java/org/apache/hive/service/cli/operation/Operation.java 6f4b8dc 
  service/src/java/org/apache/hive/service/cli/operation/OperationManager.java bcdb67f 
  service/src/java/org/apache/hive/service/cli/operation/SQLOperation.java 4ee1b74 
  service/src/java/org/apache/hive/service/cli/thrift/ThriftCLIService.java 9df110e 
  service/src/java/org/apache/hive/service/cli/thrift/ThriftCLIServiceClient.java 9bb2a0f 
  service/src/test/org/apache/hive/service/cli/CLIServiceTest.java cd9d99a 
  service/src/test/org/apache/hive/service/cli/thrift/ThriftCLIServiceTest.java ff7166d 

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


Testing
-------


Thanks,

Vaibhav Gumashta

Re: Review Request 15151: Better error reporting by async threads in HiveServer2

Posted by Vaibhav Gumashta <vg...@hortonworks.com>.

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

(Updated Nov. 11, 2013, 7:23 p.m.)


Review request for hive, Prasad Mujumdar and Thejas Nair.


Changes
-------

Lint cleanup.


Bugs: HIVE-5230
    https://issues.apache.org/jira/browse/HIVE-5230


Repository: hive-git


Description
-------

[HIVE-4617|https://issues.apache.org/jira/browse/HIVE-4617] provides support for async execution in HS2. When a background thread gets an error, currently the client can only poll for the operation state and also the error with its stacktrace is logged. However, it will be useful to provide a richer error response like thrift API does with TStatus (which is constructed while building a Thrift response object). 


Diffs (updated)
-----

  itests/hive-unit/src/test/java/org/apache/hive/service/cli/CLIServiceTest.java PRE-CREATION 
  itests/hive-unit/src/test/java/org/apache/hive/service/cli/thrift/ThriftCLIServiceTest.java PRE-CREATION 
  jdbc/src/java/org/apache/hive/jdbc/HiveStatement.java fce19bf 
  service/if/TCLIService.thrift 1f49445 
  service/src/java/org/apache/hive/service/cli/CLIService.java 8c85386 
  service/src/java/org/apache/hive/service/cli/CLIServiceClient.java 14ef54f 
  service/src/java/org/apache/hive/service/cli/EmbeddedCLIServiceClient.java 9dca874 
  service/src/java/org/apache/hive/service/cli/HiveSQLException.java 74e8b94 
  service/src/java/org/apache/hive/service/cli/ICLIService.java f647ce6 
  service/src/java/org/apache/hive/service/cli/OperationState.java 1ec6bd1 
  service/src/java/org/apache/hive/service/cli/OperationStatus.java PRE-CREATION 
  service/src/java/org/apache/hive/service/cli/operation/Operation.java 6f4b8dc 
  service/src/java/org/apache/hive/service/cli/operation/OperationManager.java bcdb67f 
  service/src/java/org/apache/hive/service/cli/operation/SQLOperation.java 4ee1b74 
  service/src/java/org/apache/hive/service/cli/thrift/ThriftCLIService.java 9df110e 
  service/src/java/org/apache/hive/service/cli/thrift/ThriftCLIServiceClient.java 9bb2a0f 
  service/src/test/org/apache/hive/service/cli/CLIServiceTest.java cd9d99a 
  service/src/test/org/apache/hive/service/cli/thrift/ThriftCLIServiceTest.java ff7166d 

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


Testing
-------


Thanks,

Vaibhav Gumashta

Re: Review Request 15151: Better error reporting by async threads in HiveServer2

Posted by Vaibhav Gumashta <vg...@hortonworks.com>.

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

(Updated Nov. 11, 2013, 10:57 a.m.)


Review request for hive, Prasad Mujumdar and Thejas Nair.


Bugs: HIVE-5230
    https://issues.apache.org/jira/browse/HIVE-5230


Repository: hive-git


Description
-------

[HIVE-4617|https://issues.apache.org/jira/browse/HIVE-4617] provides support for async execution in HS2. When a background thread gets an error, currently the client can only poll for the operation state and also the error with its stacktrace is logged. However, it will be useful to provide a richer error response like thrift API does with TStatus (which is constructed while building a Thrift response object). 


Diffs (updated)
-----

  itests/hive-unit/src/test/java/org/apache/hive/service/cli/CLIServiceTest.java PRE-CREATION 
  itests/hive-unit/src/test/java/org/apache/hive/service/cli/thrift/ThriftCLIServiceTest.java PRE-CREATION 
  jdbc/src/java/org/apache/hive/jdbc/HiveStatement.java fce19bf 
  service/if/TCLIService.thrift 1f49445 
  service/src/java/org/apache/hive/service/cli/CLIService.java 8c85386 
  service/src/java/org/apache/hive/service/cli/CLIServiceClient.java 14ef54f 
  service/src/java/org/apache/hive/service/cli/EmbeddedCLIServiceClient.java 9dca874 
  service/src/java/org/apache/hive/service/cli/HiveSQLException.java 74e8b94 
  service/src/java/org/apache/hive/service/cli/ICLIService.java f647ce6 
  service/src/java/org/apache/hive/service/cli/OperationState.java 1ec6bd1 
  service/src/java/org/apache/hive/service/cli/OperationStatus.java PRE-CREATION 
  service/src/java/org/apache/hive/service/cli/operation/Operation.java 6f4b8dc 
  service/src/java/org/apache/hive/service/cli/operation/OperationManager.java bcdb67f 
  service/src/java/org/apache/hive/service/cli/operation/SQLOperation.java 4ee1b74 
  service/src/java/org/apache/hive/service/cli/thrift/ThriftCLIService.java 9df110e 
  service/src/java/org/apache/hive/service/cli/thrift/ThriftCLIServiceClient.java 9bb2a0f 
  service/src/test/org/apache/hive/service/cli/CLIServiceTest.java cd9d99a 
  service/src/test/org/apache/hive/service/cli/thrift/ThriftCLIServiceTest.java ff7166d 

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


Testing
-------


Thanks,

Vaibhav Gumashta

Re: Review Request 15151: Better error reporting by async threads in HiveServer2

Posted by Vaibhav Gumashta <vg...@hortonworks.com>.

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

(Updated Nov. 11, 2013, 9:49 a.m.)


Review request for hive, Prasad Mujumdar and Thejas Nair.


Changes
-------

Updated patch based on the feedback.


Bugs: HIVE-5230
    https://issues.apache.org/jira/browse/HIVE-5230


Repository: hive-git


Description
-------

[HIVE-4617|https://issues.apache.org/jira/browse/HIVE-4617] provides support for async execution in HS2. When a background thread gets an error, currently the client can only poll for the operation state and also the error with its stacktrace is logged. However, it will be useful to provide a richer error response like thrift API does with TStatus (which is constructed while building a Thrift response object). 


Diffs (updated)
-----

  itests/hive-unit/src/test/java/org/apache/hive/service/cli/CLIServiceTest.java PRE-CREATION 
  itests/hive-unit/src/test/java/org/apache/hive/service/cli/thrift/ThriftCLIServiceTest.java PRE-CREATION 
  jdbc/src/java/org/apache/hive/jdbc/HiveStatement.java fce19bf 
  service/if/TCLIService.thrift 1f49445 
  service/src/java/org/apache/hive/service/cli/CLIService.java 8c85386 
  service/src/java/org/apache/hive/service/cli/CLIServiceClient.java 14ef54f 
  service/src/java/org/apache/hive/service/cli/EmbeddedCLIServiceClient.java 9dca874 
  service/src/java/org/apache/hive/service/cli/ICLIService.java f647ce6 
  service/src/java/org/apache/hive/service/cli/OperationStatus.java PRE-CREATION 
  service/src/java/org/apache/hive/service/cli/operation/Operation.java 6f4b8dc 
  service/src/java/org/apache/hive/service/cli/operation/OperationManager.java bcdb67f 
  service/src/java/org/apache/hive/service/cli/operation/SQLOperation.java 4ee1b74 
  service/src/java/org/apache/hive/service/cli/thrift/ThriftCLIService.java 9df110e 
  service/src/java/org/apache/hive/service/cli/thrift/ThriftCLIServiceClient.java 9bb2a0f 

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


Testing
-------


Thanks,

Vaibhav Gumashta

Re: Review Request 15151: Better error reporting by async threads in HiveServer2

Posted by Thejas Nair <th...@hortonworks.com>.

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



service/src/java/org/apache/hive/service/cli/thrift/ThriftCLIServiceClient.java
<https://reviews.apache.org/r/15151/#comment55179>

    Ok, I see that this keeps the behavior same as before. Can you please add a comment saying why its null ? It is not very obvious.
    



service/src/test/org/apache/hive/service/cli/CLIServiceTest.java
<https://reviews.apache.org/r/15151/#comment55180>

    The error messages should not change without a good reason. Users might be relying on it. It can change, but then tests that use it would also need to change.
    
    Checking for error message here will ensure that error is getting propagated properly.
    


- Thejas Nair


On Nov. 1, 2013, 12:54 a.m., Vaibhav Gumashta wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15151/
> -----------------------------------------------------------
> 
> (Updated Nov. 1, 2013, 12:54 a.m.)
> 
> 
> Review request for hive, Prasad Mujumdar and Thejas Nair.
> 
> 
> Bugs: HIVE-5230
>     https://issues.apache.org/jira/browse/HIVE-5230
> 
> 
> Repository: hive-git
> 
> 
> Description
> -------
> 
> [HIVE-4617|https://issues.apache.org/jira/browse/HIVE-4617] provides support for async execution in HS2. When a background thread gets an error, currently the client can only poll for the operation state and also the error with its stacktrace is logged. However, it will be useful to provide a richer error response like thrift API does with TStatus (which is constructed while building a Thrift response object). 
> 
> 
> Diffs
> -----
> 
>   service/src/java/org/apache/hive/service/cli/CLIService.java 1a7f338 
>   service/src/java/org/apache/hive/service/cli/CLIServiceClient.java 14ef54f 
>   service/src/java/org/apache/hive/service/cli/EmbeddedCLIServiceClient.java 9dca874 
>   service/src/java/org/apache/hive/service/cli/ICLIService.java f647ce6 
>   service/src/java/org/apache/hive/service/cli/OperationStatus.java PRE-CREATION 
>   service/src/java/org/apache/hive/service/cli/operation/Operation.java 6f4b8dc 
>   service/src/java/org/apache/hive/service/cli/operation/OperationManager.java bcdb67f 
>   service/src/java/org/apache/hive/service/cli/operation/SQLOperation.java f6adf92 
>   service/src/java/org/apache/hive/service/cli/thrift/ThriftCLIService.java 9df110e 
>   service/src/java/org/apache/hive/service/cli/thrift/ThriftCLIServiceClient.java 9bb2a0f 
>   service/src/test/org/apache/hive/service/cli/CLIServiceTest.java d6caed1 
>   service/src/test/org/apache/hive/service/cli/thrift/ThriftCLIServiceTest.java ff7166d 
> 
> Diff: https://reviews.apache.org/r/15151/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Vaibhav Gumashta
> 
>