> 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`? > > denys kuzmenko wrote: > it's being accesed only inside of the critical section (within the lock > boundaries)
does java guarantee that non-volatile variables accessed inside a critical section are not cached locally by a CPU? > 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? > > denys kuzmenko wrote: > queryCompleted and (lastSparkJobCompletionTime = 0) are complementary > conditions that are checked and set at the same place > > denys kuzmenko wrote: > queryCompleted and (lastSparkJobCompletionTime > 0) > > denys kuzmenko wrote: > we do have bunch of tests (TestSparkSession*, TestLocalSparkClient) that > are covering this i don't think we have a test that explicitly checks what happens when a timeout is triggered before the first HoS query is run, but i think i added some in HIVE-20519 already anyway > 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 okay, still recommend using a separate lock > 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? > > denys kuzmenko wrote: > it's not required. close() method is covered with the lock, and > activeJobs is a concurrent collection what happens if a job is submitted after `hasTimedOut` returns true? - Sahil ----------------------------------------------------------- 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 > >
