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