> On Oct. 16, 2018, 1:47 p.m., Sahil Takiar wrote: > > ql/src/java/org/apache/hadoop/hive/ql/exec/spark/LocalHiveSparkClient.java > > Line 73 (original), 73 (patched) > > <https://reviews.apache.org/r/69022/diff/2/?file=2097990#file2097990line75> > > > > if we expect multiple sessions to access this, should we make this > > `volatile`?
it's being accesed only inside of the critical section (within the lock boundaries) > On Oct. 16, 2018, 1:47 p.m., Sahil Takiar wrote: > > ql/src/java/org/apache/hadoop/hive/ql/exec/spark/LocalHiveSparkClient.java > > Lines 74 (patched) > > <https://reviews.apache.org/r/69022/diff/2/?file=2097990#file2097990line76> > > > > should probably make this `volatile` in case multiple threads try to > > get an instance it's being accesed only inside of the critical section (within the lock boundaries) > On Oct. 16, 2018, 1:47 p.m., Sahil Takiar wrote: > > ql/src/java/org/apache/hadoop/hive/ql/exec/spark/session/SparkSessionImpl.java > > Lines 112-116 (original) > > <https://reviews.apache.org/r/69022/diff/2/?file=2097991#file2097991line112> > > > > do we have unit tests that cover this? queryCompleted and (lastSparkJobCompletionTime = 0) are complementary conditions that are checked and set at the same place > 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. i think it should be addressed in another JIRA, right now we need to have working at least basic use-case > 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 352 (original) > > <https://reviews.apache.org/r/69022/diff/2/?file=2097991#file2097991line361> > > > > why remove this? it's not required. close() method is covered with the lock, and activeJobs is a concurrent collection - denys ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69022/#review209617 ----------------------------------------------------------- On Oct. 15, 2018, 7:21 p.m., denys kuzmenko wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/69022/ > ----------------------------------------------------------- > > (Updated Oct. 15, 2018, 7:21 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/2/ > > > 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 > >
