Mailing-List: contact dev-help@hive.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@hive.apache.org
Content-Type: multipart/alternative;
 boundary="===============7018408158456762080=="
MIME-Version: 1.0
Subject: Re: Review Request 15151: Better error reporting by async threads in
 HiveServer2
From: "Thejas Nair" <thejas@hortonworks.com>
To: "Prasad Mujumdar" <prasadm@cloudera.com>,
 "Thejas Nair" <thejas@hortonworks.com>
Cc: "Vaibhav Gumashta" <vgumashta@hortonworks.com>,
 "hive" <dev@hive.apache.org>
Date: Thu, 07 Nov 2013 03:33:16 -0000
Message-ID: <20131107033316.30022.90717@reviews.apache.org>
Auto-Submitted: auto-generated
Sender: "Thejas Nair" <noreply@reviews.apache.org>
References: <20131105033227.30022.87418@reviews.apache.org>
In-Reply-To: <20131105033227.30022.87418@reviews.apache.org>
Reply-To: "Thejas Nair" <thejas@hortonworks.com>

--===============7018408158456762080==
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit


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


--===============7018408158456762080==--