> On Oct. 16, 2018, 1:47 p.m., Sahil Takiar wrote: > > ql/src/java/org/apache/hadoop/hive/ql/exec/spark/session/SparkSessionImpl.java > > Line 125 (original), 120 (patched) > > <https://reviews.apache.org/r/69022/diff/2/?file=2097991#file2097991line125> > > > > we might need to re-think how we are synchronizing this method a bit. I > > think we want to support the use case where we call `close()` while > > `open()` is being run. The offers a way for the user to cancel a session > > while it is being opened, which can be useful if opening a session takes a > > long time, which can happen on a busy cluster where there aren't enough > > resources to open a session. > > > > fixing that might be out of the scope of this JIRA, so I would > > recommend using a separate lock to guard against multiple users calling > > open on the same session. > > Sahil Takiar wrote: > Tracking the aformentioned fix in HIVE-20519, unless you want to fix it > in this patch. > > denys kuzmenko wrote: > i think it should be addressed in another JIRA, right now we need to have > working at least basic use-case > > Sahil Takiar wrote: > okay, still recommend using a separate lock > > denys kuzmenko wrote: > open() and close() both manipulate with the shared variable (isOpen), so > they have to be synchronized on a same monitor (at least in current approach). > I am not sure whether SparkContext supports instant interruption > (Thread.interrupt or sc.stop()). However when closing session that is in > progress, you have to take care of SparkContext. > > Sahil Takiar wrote: > yes, but we need to support calling `close()` while `open()` is being > run, as I described in my first comment. the (2) bullet point in the RB > description states that you want to guard against multiple callers invoking > the `open()` method, so logically you should just have a single lock to > handle this behavior. i suggest you use a separate lock for this scenario > because the `closeLock` is meant to handle synchronization of the `close()` > method, it was not meant to handle synchronization of the `open()` method by > multiple callers. IMO by re-using the lock we make the code harder to > understand because we are using the `closeLock` for functionality that should > be outside its scope. > > I don't feel strongly about this though given we will need to re-factor > this code later anyway to support calling `close()` while `open()` is > running, so I'll leave it up to you.
Let's have a seperate JIRA for this use-case. - denys ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69022/#review209617 ----------------------------------------------------------- On Oct. 16, 2018, 4:49 p.m., denys kuzmenko wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/69022/ > ----------------------------------------------------------- > > (Updated Oct. 16, 2018, 4:49 p.m.) > > > Review request for hive, Sahil Takiar and Adam Szita. > > > Bugs: HIVE-20737 > https://issues.apache.org/jira/browse/HIVE-20737 > > > Repository: hive-git > > > Description > ------- > > 1. Local SparkContext is shared between user sessions and should be closed > only when there is no active. > 2. Possible race condition in SparkSession.open() in case when user queries > run in parallel within the same session. > > > Diffs > ----- > > ql/src/java/org/apache/hadoop/hive/ql/exec/spark/LocalHiveSparkClient.java > 72ff53e3bd > > ql/src/java/org/apache/hadoop/hive/ql/exec/spark/session/SparkSessionImpl.java > bb50129518 > > ql/src/test/org/apache/hadoop/hive/ql/exec/spark/TestLocalHiveSparkClient.java > PRE-CREATION > > > Diff: https://reviews.apache.org/r/69022/diff/3/ > > > Testing > ------- > > Added TestLocalHiveSparkClient test > > > File Attachments > ---------------- > > HIVE-20737.7.patch > > https://reviews.apache.org/media/uploaded/files/2018/10/15/9cf8a2b3-9ec1-4316-81d0-3cd124b1a9fd__HIVE-20737.7.patch > > > Thanks, > > denys kuzmenko > >
