> 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. > > Marcelo Vanzin wrote: > 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.
I see. So the shared state of the job handler consists of state, listeners, and sparkJobIds, which needs to be protected. Thus, I'd suggest we change synchronize(listeners) to synchronized(this) or declare the method as "synchronized". No essential difference, but for better clarity. - Xuefu ----------------------------------------------------------- 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 > >
