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