hive-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Marcelo Vanzin" <vanzin+...@cloudera.com>
Subject Re: Review Request 29954: HIVE-9179. Add listener API to JobHandle.
Date Sat, 17 Jan 2015 00:30:08 GMT


> On Jan. 17, 2015, 12:19 a.m., Xuefu Zhang wrote:
> > spark-client/src/main/java/org/apache/hive/spark/client/JobHandleImpl.java, line
179
> > <https://reviews.apache.org/r/29954/diff/1-2/?file=823286#file823286line179>
> >
> >     Sorry I didn't get it, but why?
> >     Clarity but not perf is my concern. Here we are notifying listeners with a new
Spark job ID, which is done in the for loop, which is synchronized. This means no listener
may be added or removed from the listeners. On the other hand, sparkJobIds.add(sparkJobId)
seems irrelevant to any changes to listeners, unless I missed anything. I don't understand
why either of the two cases might happen as you suggested.

Threads: T1 updating the job handle, T2 adding a listener

Case 1:
   Statement 1 (S1): sparkJobIds.add(sparkJobId);
   Statement 2 (S2): synchronized (listeners) { /* call onSparkJobStarted(newSparkJobId) on
every listener */ }

Timeline:
T1: executes S1
T2: calls addListener(), new listener is notified of the sparkJobId added above
T1: executes S2. New listener is notified again of new spark job ID.


Case 2:
  Invert S1 and S2.
  
T2: calls addListener()
T1: executes S1. Listener is called with the current state of the handle and new Spark job
ID. Listener checks `handle.getSparkJobIDs().contains(newSparkJobId)`, check fails.


Those seem pretty easy to understand to me. The current code avoids both of them.


- Marcelo


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29954/#review68513
-----------------------------------------------------------


On Jan. 16, 2015, 11:24 p.m., Marcelo Vanzin wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29954/
> -----------------------------------------------------------
> 
> (Updated Jan. 16, 2015, 11:24 p.m.)
> 
> 
> Review request for hive, Brock Noland, chengxiang li, and Xuefu Zhang.
> 
> 
> Bugs: HIVE-9179
>     https://issues.apache.org/jira/browse/HIVE-9179
> 
> 
> Repository: hive-git
> 
> 
> Description
> -------
> 
> HIVE-9179. Add listener API to JobHandle.
> 
> 
> Diffs
> -----
> 
>   spark-client/pom.xml 77016df61a0bcbd94058bcbd2825c6c210a70e14 
>   spark-client/src/main/java/org/apache/hive/spark/client/BaseProtocol.java f9c10b196ab47b5b4f4c0126ad455869ab68f0ca

>   spark-client/src/main/java/org/apache/hive/spark/client/JobHandle.java e760ce35d92bedf4d301b08ec57d1c2dc37a39f0

>   spark-client/src/main/java/org/apache/hive/spark/client/JobHandleImpl.java 1b8feedb0b23aa7897dc6ac37ea5c0209e71d573

>   spark-client/src/main/java/org/apache/hive/spark/client/RemoteDriver.java 0d49ed3d9e33ca08d6a7526c1c434a0dd0a06a67

>   spark-client/src/main/java/org/apache/hive/spark/client/SparkClientImpl.java a30d8cbbaae9d25b1cffdc286b546f549e439545

>   spark-client/src/test/java/org/apache/hive/spark/client/TestJobHandle.java PRE-CREATION

>   spark-client/src/test/java/org/apache/hive/spark/client/TestSparkClient.java 795d62c776cec5e9da2a24b7d40bc749a03186ab

> 
> Diff: https://reviews.apache.org/r/29954/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Marcelo Vanzin
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message