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