> 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. > > Xuefu Zhang wrote: > 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.
The synchronization is *only* needed because of the listeners. It's there so that when you add a listener, you never miss an event - if they didn't exist, you wouldn't need any synchronization anywhere in this class. So it makes better sense to synchronize on the listeners. - 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 > >
