Re: Review Request 69022: HIVE-20737: Local SparkContext is shared between user sessions and should be closed only when there is no active

2018-10-18 Thread denys kuzmenko via Review Board 


> 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)
> > 
> >
> > 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
> 
>

Re: Review Request 69022: HIVE-20737: Local SparkContext is shared between user sessions and should be closed only when there is no active

2018-10-18 Thread denys kuzmenko via Review Board 


> 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)
> > 
> >
> > why remove this?
> 
> denys kuzmenko wrote:
> it's not required. close() method is covered with the lock, and 
> activeJobs is a concurrent collection
> 
> Sahil Takiar wrote:
> what happens if a job is submitted after `hasTimedOut` returns true?
> 
> denys kuzmenko wrote:
> I see. However existing lock won't help as it doesn't prevent other 
> threads from adding new queries. 
> 
> public void onQuerySubmission(String queryId) {
> activeJobs.add(queryId);
> }
> 
> we might need to cover this with separate lock (onQueryCompletion, 
> onQuerySubmission, triggerTimeout)
> What do you think?
> 
> denys kuzmenko wrote:
> what happens if a job is submitted after hasTimedOut returns true? 
> current Session will be closed and a new one will be opened.
> 
> denys kuzmenko wrote:
> there might be an issue when 2nd session checkes state and get isOpen and 
> before it's reaching the submit phase, 1st one calls the close.
> I think we need synchronization for active sessions manipulation.
> 
> denys kuzmenko wrote:
> fixed. reverted back to the original locking. Above tricky case will be 
> handled by preventing new queries to execute open() before close() is 
> complete.
> 
> denys kuzmenko wrote:
> @before triggerTimeout() is complete
> 
> Sahil Takiar wrote:
> A little confused by all the comments here. Looks like the change has 
> been reverted. Do we agree that the current code is sufficient, or are their 
> other edge cases you think need to be covered?

Sorry for that, that was just my thoughts. 
I think, current code is sufficient for existing use case, however it's 
definetely not designed to support case where close() is called and could be 
executed while open() is being run


- 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
> 
>

Re: Review Request 69022: HIVE-20737: Local SparkContext is shared between user sessions and should be closed only when there is no active

2018-10-18 Thread Sahil Takiar 


> 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)
> > 
> >
> > 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)
> 
> Sahil Takiar wrote:
> does java guarantee that non-volatile variables accessed inside a 
> critical section are not cached locally by a CPU?
> 
> denys kuzmenko wrote:
> In short - yes.
> 
> JSR 133 (Java Memory Model)
> 
> Synchronization ensures that memory writes by a thread before or during a 
> synchronized block are made visible in a predictable manner to other threads 
> which synchronize on the same monitor. After we exit a synchronized block, we 
> release the monitor, which has the effect of flushing the cache to main 
> memory, so that writes made by this thread can be visible to other threads. 
> Before we can enter a synchronized block, we acquire the monitor, which has 
> the effect of invalidating the local processor cache so that variables will 
> be reloaded from main memory. We will then be able to see all of the writes 
> made visible by the previous release.

makes sense


> 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)
> > 
> >
> > 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.

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.


> 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)
> > 
> >
> > why remove this?
> 
> denys kuzmenko wrote:
> it's not required. close() method is covered with the lock, and 
> activeJobs is a concurrent collection
> 
> Sahil Takiar wrote:
> what happens if a job is submitted after `hasTimedOut` returns true?
> 
> denys kuzmenko wrote:
> I see. However existing lock won't help as it doesn't prevent other 
> threads from adding new queries. 
> 
> public void onQuerySubmission(String queryId) {
> activeJobs.add(queryId);
> }
> 
> we might need to cover this with separate lock (onQueryCompletion, 
> onQuerySubmission, triggerTimeout)
> What do you think?
> 
> denys kuzmenko wrote:
> what happens if a job is submitted after hasTimedOut returns true? 
> current Session will be closed and a new one will be opened.
> 
> denys kuzmenko wrote:
> there might be an issue when 2nd

Re: Review Request 69022: HIVE-20737: Local SparkContext is shared between user sessions and should be closed only when there is no active

2018-10-18 Thread Antal Sinkovits via Review Board 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69022/#review209739
---


Ship it!




Ship It!

- Antal Sinkovits


On okt. 16, 2018, 4:49 du, denys kuzmenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69022/
> ---
> 
> (Updated okt. 16, 2018, 4:49 du)
> 
> 
> 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
> 
>

Re: Review Request 69022: HIVE-20737: Local SparkContext is shared between user sessions and should be closed only when there is no active

2018-10-16 Thread denys kuzmenko via Review Board 


> 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)
> > 
> >
> > 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

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.


- 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
> 
>

Re: Review Request 69022: HIVE-20737: Local SparkContext is shared between user sessions and should be closed only when there is no active

2018-10-16 Thread denys kuzmenko via Review Board 


> 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)
> > 
> >
> > 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)
> 
> Sahil Takiar wrote:
> does java guarantee that non-volatile variables accessed inside a 
> critical section are not cached locally by a CPU?

In short - yes.

JSR 133 (Java Memory Model)

Synchronization ensures that memory writes by a thread before or during a 
synchronized block are made visible in a predictable manner to other threads 
which synchronize on the same monitor. After we exit a synchronized block, we 
release the monitor, which has the effect of flushing the cache to main memory, 
so that writes made by this thread can be visible to other threads. Before we 
can enter a synchronized block, we acquire the monitor, which has the effect of 
invalidating the local processor cache so that variables will be reloaded from 
main memory. We will then be able to see all of the writes made visible by the 
previous release.


- 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
> 
>

Re: Review Request 69022: HIVE-20737: Local SparkContext is shared between user sessions and should be closed only when there is no active

2018-10-16 Thread denys kuzmenko via Review Board 


> 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)
> > 
> >
> > why remove this?
> 
> denys kuzmenko wrote:
> it's not required. close() method is covered with the lock, and 
> activeJobs is a concurrent collection
> 
> Sahil Takiar wrote:
> what happens if a job is submitted after `hasTimedOut` returns true?
> 
> denys kuzmenko wrote:
> I see. However existing lock won't help as it doesn't prevent other 
> threads from adding new queries. 
> 
> public void onQuerySubmission(String queryId) {
> activeJobs.add(queryId);
> }
> 
> we might need to cover this with separate lock (onQueryCompletion, 
> onQuerySubmission, triggerTimeout)
> What do you think?
> 
> denys kuzmenko wrote:
> what happens if a job is submitted after hasTimedOut returns true? 
> current Session will be closed and a new one will be opened.
> 
> denys kuzmenko wrote:
> there might be an issue when 2nd session checkes state and get isOpen and 
> before it's reaching the submit phase, 1st one calls the close.
> I think we need synchronization for active sessions manipulation.
> 
> denys kuzmenko wrote:
> fixed. reverted back to the original locking. Above tricky case will be 
> handled by preventing new queries to execute open() before close() is 
> complete.

@before triggerTimeout() is complete


- 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
> 
>

Re: Review Request 69022: HIVE-20737: Local SparkContext is shared between user sessions and should be closed only when there is no active

2018-10-16 Thread denys kuzmenko via Review Board 

---
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 (updated)
-

  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/

Changes: https://reviews.apache.org/r/69022/diff/2-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

Re: Review Request 69022: HIVE-20737: Local SparkContext is shared between user sessions and should be closed only when there is no active

2018-10-16 Thread denys kuzmenko via Review Board 


> 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)
> > 
> >
> > why remove this?
> 
> denys kuzmenko wrote:
> it's not required. close() method is covered with the lock, and 
> activeJobs is a concurrent collection
> 
> Sahil Takiar wrote:
> what happens if a job is submitted after `hasTimedOut` returns true?
> 
> denys kuzmenko wrote:
> I see. However existing lock won't help as it doesn't prevent other 
> threads from adding new queries. 
> 
> public void onQuerySubmission(String queryId) {
> activeJobs.add(queryId);
> }
> 
> we might need to cover this with separate lock (onQueryCompletion, 
> onQuerySubmission, triggerTimeout)
> What do you think?
> 
> denys kuzmenko wrote:
> what happens if a job is submitted after hasTimedOut returns true? 
> current Session will be closed and a new one will be opened.
> 
> denys kuzmenko wrote:
> there might be an issue when 2nd session checkes state and get isOpen and 
> before it's reaching the submit phase, 1st one calls the close.
> I think we need synchronization for active sessions manipulation.

fixed. reverted back to the original locking. Above tricky case will be handled 
by preventing new queries to execute open() before close() is complete.


- 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
> 
>

Re: Review Request 69022: HIVE-20737: Local SparkContext is shared between user sessions and should be closed only when there is no active

2018-10-16 Thread denys kuzmenko via Review Board 


> 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)
> > 
> >
> > why remove this?
> 
> denys kuzmenko wrote:
> it's not required. close() method is covered with the lock, and 
> activeJobs is a concurrent collection
> 
> Sahil Takiar wrote:
> what happens if a job is submitted after `hasTimedOut` returns true?
> 
> denys kuzmenko wrote:
> I see. However existing lock won't help as it doesn't prevent other 
> threads from adding new queries. 
> 
> public void onQuerySubmission(String queryId) {
> activeJobs.add(queryId);
> }
> 
> we might need to cover this with separate lock (onQueryCompletion, 
> onQuerySubmission, triggerTimeout)
> What do you think?
> 
> denys kuzmenko wrote:
> what happens if a job is submitted after hasTimedOut returns true? 
> current Session will be closed and a new one will be opened.

there might be an issue when 2nd session checkes state and get isOpen and 
before it's reaching the submit phase, 1st one calls the close.
I think we need synchronization for active sessions manipulation.


- 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
> 
>

Re: Review Request 69022: HIVE-20737: Local SparkContext is shared between user sessions and should be closed only when there is no active

2018-10-16 Thread denys kuzmenko via Review Board 


> 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)
> > 
> >
> > why remove this?
> 
> denys kuzmenko wrote:
> it's not required. close() method is covered with the lock, and 
> activeJobs is a concurrent collection
> 
> Sahil Takiar wrote:
> what happens if a job is submitted after `hasTimedOut` returns true?
> 
> denys kuzmenko wrote:
> I see. However existing lock won't help as it doesn't prevent other 
> threads from adding new queries. 
> 
> public void onQuerySubmission(String queryId) {
> activeJobs.add(queryId);
> }
> 
> we might need to cover this with separate lock (onQueryCompletion, 
> onQuerySubmission, triggerTimeout)
> What do you think?

what happens if a job is submitted after hasTimedOut returns true? current 
Session will be closed and a new one will be opened.


- 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
> 
>

Re: Review Request 69022: HIVE-20737: Local SparkContext is shared between user sessions and should be closed only when there is no active

2018-10-16 Thread denys kuzmenko via Review Board 


> 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)
> > 
> >
> > why remove this?
> 
> denys kuzmenko wrote:
> it's not required. close() method is covered with the lock, and 
> activeJobs is a concurrent collection
> 
> Sahil Takiar wrote:
> what happens if a job is submitted after `hasTimedOut` returns true?

I see. However existing lock won't help as it doesn't prevent other threads 
from adding new queries. 

public void onQuerySubmission(String queryId) {
activeJobs.add(queryId);
}

we might need to cover this with separate lock (onQueryCompletion, 
onQuerySubmission, triggerTimeout)
What do you think?


- 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
> 
>

Re: Review Request 69022: HIVE-20737: Local SparkContext is shared between user sessions and should be closed only when there is no active

2018-10-16 Thread Sahil Takiar 


> 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)
> > 
> >
> > 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)
> > 
> >
> > 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)
> > 
> >
> > 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)
> > 
> >
> > 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
> 
>

Re: Review Request 69022: HIVE-20737: Local SparkContext is shared between user sessions and should be closed only when there is no active

2018-10-16 Thread denys kuzmenko via Review Board 


> 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)
> > 
> >
> > 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)

we do have bunch of tests (TestSparkSession*, TestLocalSparkClient) that are 
covering this


- 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
> 
>

Re: Review Request 69022: HIVE-20737: Local SparkContext is shared between user sessions and should be closed only when there is no active

2018-10-16 Thread denys kuzmenko via Review Board 


> 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)
> > 
> >
> > 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

queryCompleted and (lastSparkJobCompletionTime > 0)


- 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
> 
>

Re: Review Request 69022: HIVE-20737: Local SparkContext is shared between user sessions and should be closed only when there is no active

2018-10-16 Thread denys kuzmenko via Review Board 


> 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)
> > 
> >
> > 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)
> > 
> >
> > 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)
> > 
> >
> > 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)
> > 
> >
> > 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)
> > 
> >
> > 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
> 
>

Re: Review Request 69022: HIVE-20737: Local SparkContext is shared between user sessions and should be closed only when there is no active

2018-10-16 Thread Sahil Takiar 


> 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)
> > 
> >
> > 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.

Tracking the aformentioned fix in HIVE-20519, unless you want to fix it in this 
patch.


- 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
> 
>

Re: Review Request 69022: HIVE-20737: Local SparkContext is shared between user sessions and should be closed only when there is no active

2018-10-16 Thread Sahil Takiar 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69022/#review209617
---




ql/src/java/org/apache/hadoop/hive/ql/exec/spark/LocalHiveSparkClient.java
Line 73 (original), 73 (patched)


if we expect multiple sessions to access this, should we make this 
`volatile`?



ql/src/java/org/apache/hadoop/hive/ql/exec/spark/LocalHiveSparkClient.java
Lines 74 (patched)


should probably make this `volatile` in case multiple threads try to get an 
instance



ql/src/java/org/apache/hadoop/hive/ql/exec/spark/session/SparkSessionImpl.java
Lines 112-116 (original)


do we have unit tests that cover this?



ql/src/java/org/apache/hadoop/hive/ql/exec/spark/session/SparkSessionImpl.java
Line 125 (original), 120 (patched)


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.



ql/src/java/org/apache/hadoop/hive/ql/exec/spark/session/SparkSessionImpl.java
Line 352 (original)


why remove this?


- Sahil Takiar


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
> 
>

Re: Review Request 69022: HIVE-20737: Local SparkContext is shared between user sessions and should be closed only when there is no active

2018-10-15 Thread denys kuzmenko via Review Board 

---
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.


Changes
---

updated the patch file.


Summary (updated)
-

HIVE-20737: Local SparkContext is shared between user sessions and should be 
closed only when there is no active


Bugs: HIVE-20737
https://issues.apache.org/jira/browse/HIVE-20737


Repository: hive-git


Description (updated)
---

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