Home | About | Sematext search-lucene.com search-hadoop.com
 Search Hadoop and all its subprojects:

Switch to Threaded View
Hive, mail # dev - Review Request 15151: Better error reporting by async threads in HiveServer2


Copy link to this message
-
Re: Review Request 15151: Better error reporting by async threads in HiveServer2
Vaibhav Gumashta 2013-11-05, 20:08


> 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).