You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hive.apache.org by Venki Korukanti <ve...@gmail.com> on 2014/08/16 03:59:47 UTC
Review Request 24763: HIVE-7606: Design SparkSession,
SparkSessionManager
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24763/
-----------------------------------------------------------
Review request for hive, Brock Noland and Szehon Ho.
Repository: hive-git
Description
-------
Please see JIRA HIVE-7606 for description
Diffs
-----
ql/src/java/org/apache/hadoop/hive/ql/exec/spark/SparkTask.java 5ac5a25
ql/src/java/org/apache/hadoop/hive/ql/exec/spark/session/SparkSession.java PRE-CREATION
ql/src/java/org/apache/hadoop/hive/ql/exec/spark/session/SparkSessionImpl.java PRE-CREATION
ql/src/java/org/apache/hadoop/hive/ql/exec/spark/session/SparkSessionManager.java PRE-CREATION
ql/src/java/org/apache/hadoop/hive/ql/exec/spark/session/SparkSessionManagerImpl.java PRE-CREATION
ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java fcfcf42
ql/src/test/org/apache/hadoop/hive/ql/exec/spark/session/TestSparkSessionManagerImpl.java PRE-CREATION
service/src/java/org/apache/hive/service/server/HiveServer2.java 0864dfb
Diff: https://reviews.apache.org/r/24763/diff/
Testing
-------
Added unittests to test SessionManagerImpl in single session mode (HiveCLI) and multi-session mode (HiveServer2). Also tested few queries from Hive CLI. Testing using actual testing is blocked due to HIVE-7747.
Thanks,
Venki Korukanti
Re: Review Request 24763: HIVE-7606: Design SparkSession,
SparkSessionManager
Posted by Brock Noland <br...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24763/#review50917
-----------------------------------------------------------
Ship it!
Ship It!
- Brock Noland
On Aug. 17, 2014, 10 p.m., Venki Korukanti wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24763/
> -----------------------------------------------------------
>
> (Updated Aug. 17, 2014, 10 p.m.)
>
>
> Review request for hive, Brock Noland and Szehon Ho.
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> Please see JIRA HIVE-7606 for description
>
>
> Diffs
> -----
>
> ql/src/java/org/apache/hadoop/hive/ql/exec/spark/SparkTask.java 5ac5a25
> ql/src/java/org/apache/hadoop/hive/ql/exec/spark/session/SparkSession.java PRE-CREATION
> ql/src/java/org/apache/hadoop/hive/ql/exec/spark/session/SparkSessionImpl.java PRE-CREATION
> ql/src/java/org/apache/hadoop/hive/ql/exec/spark/session/SparkSessionManager.java PRE-CREATION
> ql/src/java/org/apache/hadoop/hive/ql/exec/spark/session/SparkSessionManagerImpl.java PRE-CREATION
> ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java 9798cf3
> ql/src/test/org/apache/hadoop/hive/ql/exec/spark/session/TestSparkSessionManagerImpl.java PRE-CREATION
> service/src/java/org/apache/hive/service/server/HiveServer2.java 0864dfb
>
> Diff: https://reviews.apache.org/r/24763/diff/
>
>
> Testing
> -------
>
> Added unittests to test SessionManagerImpl in single session mode (HiveCLI) and multi-session mode (HiveServer2). Also tested few queries from Hive CLI. Testing using actual HiveServer2 is blocked due to HIVE-7747.
>
>
> Thanks,
>
> Venki Korukanti
>
>
Re: Review Request 24763: HIVE-7606: Design SparkSession,
SparkSessionManager
Posted by Venki Korukanti <ve...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24763/
-----------------------------------------------------------
(Updated Aug. 17, 2014, 10 p.m.)
Review request for hive, Brock Noland and Szehon Ho.
Changes
-------
Includes review changes.
Repository: hive-git
Description
-------
Please see JIRA HIVE-7606 for description
Diffs (updated)
-----
ql/src/java/org/apache/hadoop/hive/ql/exec/spark/SparkTask.java 5ac5a25
ql/src/java/org/apache/hadoop/hive/ql/exec/spark/session/SparkSession.java PRE-CREATION
ql/src/java/org/apache/hadoop/hive/ql/exec/spark/session/SparkSessionImpl.java PRE-CREATION
ql/src/java/org/apache/hadoop/hive/ql/exec/spark/session/SparkSessionManager.java PRE-CREATION
ql/src/java/org/apache/hadoop/hive/ql/exec/spark/session/SparkSessionManagerImpl.java PRE-CREATION
ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java 9798cf3
ql/src/test/org/apache/hadoop/hive/ql/exec/spark/session/TestSparkSessionManagerImpl.java PRE-CREATION
service/src/java/org/apache/hive/service/server/HiveServer2.java 0864dfb
Diff: https://reviews.apache.org/r/24763/diff/
Testing
-------
Added unittests to test SessionManagerImpl in single session mode (HiveCLI) and multi-session mode (HiveServer2). Also tested few queries from Hive CLI. Testing using actual HiveServer2 is blocked due to HIVE-7747.
Thanks,
Venki Korukanti
Re: Review Request 24763: HIVE-7606: Design SparkSession,
SparkSessionManager
Posted by Venki Korukanti <ve...@gmail.com>.
> On Aug. 17, 2014, 6:10 p.m., Brock Noland wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/exec/spark/session/SparkSessionManagerImpl.java, line 177
> > <https://reviews.apache.org/r/24763/diff/1/?file=661979#file661979line177>
> >
> > createdSessions.clear()?
Added createdSessions.clear()
> On Aug. 17, 2014, 6:10 p.m., Brock Noland wrote:
> > ql/src/test/org/apache/hadoop/hive/ql/exec/spark/session/TestSparkSessionManagerImpl.java, line 32
> > <https://reviews.apache.org/r/24763/diff/1/?file=661981#file661981line32>
> >
> > Random is threadsafe on the Oracle JVM but it's not guranteed to be thread safe by the spec. Should we move this class into the inner class so our IBM friends won't have to fix this, if their version is not thread safe?
Moved inside the Run method.
> On Aug. 17, 2014, 6:10 p.m., Brock Noland wrote:
> > ql/src/test/org/apache/hadoop/hive/ql/exec/spark/session/TestSparkSessionManagerImpl.java, line 77
> > <https://reviews.apache.org/r/24763/diff/1/?file=661981#file661981line77>
> >
> > Should we use a logger instead?
Added a logger. Currently only error messages go to the Logger. Tests checkpoint/msgs still print to stdout that way hive.log is clean.
- Venki
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24763/#review50843
-----------------------------------------------------------
On Aug. 16, 2014, 2:02 a.m., Venki Korukanti wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24763/
> -----------------------------------------------------------
>
> (Updated Aug. 16, 2014, 2:02 a.m.)
>
>
> Review request for hive, Brock Noland and Szehon Ho.
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> Please see JIRA HIVE-7606 for description
>
>
> Diffs
> -----
>
> ql/src/java/org/apache/hadoop/hive/ql/exec/spark/SparkTask.java 5ac5a25
> ql/src/java/org/apache/hadoop/hive/ql/exec/spark/session/SparkSession.java PRE-CREATION
> ql/src/java/org/apache/hadoop/hive/ql/exec/spark/session/SparkSessionImpl.java PRE-CREATION
> ql/src/java/org/apache/hadoop/hive/ql/exec/spark/session/SparkSessionManager.java PRE-CREATION
> ql/src/java/org/apache/hadoop/hive/ql/exec/spark/session/SparkSessionManagerImpl.java PRE-CREATION
> ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java fcfcf42
> ql/src/test/org/apache/hadoop/hive/ql/exec/spark/session/TestSparkSessionManagerImpl.java PRE-CREATION
> service/src/java/org/apache/hive/service/server/HiveServer2.java 0864dfb
>
> Diff: https://reviews.apache.org/r/24763/diff/
>
>
> Testing
> -------
>
> Added unittests to test SessionManagerImpl in single session mode (HiveCLI) and multi-session mode (HiveServer2). Also tested few queries from Hive CLI. Testing using actual HiveServer2 is blocked due to HIVE-7747.
>
>
> Thanks,
>
> Venki Korukanti
>
>
Re: Review Request 24763: HIVE-7606: Design SparkSession,
SparkSessionManager
Posted by Brock Noland <br...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24763/#review50843
-----------------------------------------------------------
This looks awesome! Thank you so much writing clean and obvious code! I have a few minor comments below.
ql/src/java/org/apache/hadoop/hive/ql/exec/spark/SparkTask.java
<https://reviews.apache.org/r/24763/#comment88700>
sparkSessionManager could also be null here
ql/src/java/org/apache/hadoop/hive/ql/exec/spark/session/SparkSessionImpl.java
<https://reviews.apache.org/r/24763/#comment88698>
nit: false is the default initialization value for boolean
ql/src/java/org/apache/hadoop/hive/ql/exec/spark/session/SparkSessionImpl.java
<https://reviews.apache.org/r/24763/#comment88699>
nit: can be final
ql/src/java/org/apache/hadoop/hive/ql/exec/spark/session/SparkSessionManagerImpl.java
<https://reviews.apache.org/r/24763/#comment88697>
bump to INFO?
ql/src/java/org/apache/hadoop/hive/ql/exec/spark/session/SparkSessionManagerImpl.java
<https://reviews.apache.org/r/24763/#comment88693>
Seems like rare event, I'd move this to INFO
ql/src/java/org/apache/hadoop/hive/ql/exec/spark/session/SparkSessionManagerImpl.java
<https://reviews.apache.org/r/24763/#comment88692>
createdSessions.clear()?
ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java
<https://reviews.apache.org/r/24763/#comment88695>
Let's include the exception
ql/src/test/org/apache/hadoop/hive/ql/exec/spark/session/TestSparkSessionManagerImpl.java
<https://reviews.apache.org/r/24763/#comment88702>
Random is threadsafe on the Oracle JVM but it's not guranteed to be thread safe by the spec. Should we move this class into the inner class so our IBM friends won't have to fix this, if their version is not thread safe?
ql/src/test/org/apache/hadoop/hive/ql/exec/spark/session/TestSparkSessionManagerImpl.java
<https://reviews.apache.org/r/24763/#comment88703>
Should we use a logger instead?
ql/src/test/org/apache/hadoop/hive/ql/exec/spark/session/TestSparkSessionManagerImpl.java
<https://reviews.apache.org/r/24763/#comment88704>
Let's give a basic message "interruppted during test" etc
ql/src/test/org/apache/hadoop/hive/ql/exec/spark/session/TestSparkSessionManagerImpl.java
<https://reviews.apache.org/r/24763/#comment88701>
nit: spaces
space after for and betweek = and <
service/src/java/org/apache/hive/service/server/HiveServer2.java
<https://reviews.apache.org/r/24763/#comment88696>
Thank you very much for using the log!!
I know all the code above prints stack trace with printStackTrace but let's not since we are logging to the stack trace to the log.
A
- Brock Noland
On Aug. 16, 2014, 2:02 a.m., Venki Korukanti wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24763/
> -----------------------------------------------------------
>
> (Updated Aug. 16, 2014, 2:02 a.m.)
>
>
> Review request for hive, Brock Noland and Szehon Ho.
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> Please see JIRA HIVE-7606 for description
>
>
> Diffs
> -----
>
> ql/src/java/org/apache/hadoop/hive/ql/exec/spark/SparkTask.java 5ac5a25
> ql/src/java/org/apache/hadoop/hive/ql/exec/spark/session/SparkSession.java PRE-CREATION
> ql/src/java/org/apache/hadoop/hive/ql/exec/spark/session/SparkSessionImpl.java PRE-CREATION
> ql/src/java/org/apache/hadoop/hive/ql/exec/spark/session/SparkSessionManager.java PRE-CREATION
> ql/src/java/org/apache/hadoop/hive/ql/exec/spark/session/SparkSessionManagerImpl.java PRE-CREATION
> ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java fcfcf42
> ql/src/test/org/apache/hadoop/hive/ql/exec/spark/session/TestSparkSessionManagerImpl.java PRE-CREATION
> service/src/java/org/apache/hive/service/server/HiveServer2.java 0864dfb
>
> Diff: https://reviews.apache.org/r/24763/diff/
>
>
> Testing
> -------
>
> Added unittests to test SessionManagerImpl in single session mode (HiveCLI) and multi-session mode (HiveServer2). Also tested few queries from Hive CLI. Testing using actual HiveServer2 is blocked due to HIVE-7747.
>
>
> Thanks,
>
> Venki Korukanti
>
>
Re: Review Request 24763: HIVE-7606: Design SparkSession,
SparkSessionManager
Posted by Venki Korukanti <ve...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24763/
-----------------------------------------------------------
(Updated Aug. 16, 2014, 2:02 a.m.)
Review request for hive, Brock Noland and Szehon Ho.
Repository: hive-git
Description
-------
Please see JIRA HIVE-7606 for description
Diffs
-----
ql/src/java/org/apache/hadoop/hive/ql/exec/spark/SparkTask.java 5ac5a25
ql/src/java/org/apache/hadoop/hive/ql/exec/spark/session/SparkSession.java PRE-CREATION
ql/src/java/org/apache/hadoop/hive/ql/exec/spark/session/SparkSessionImpl.java PRE-CREATION
ql/src/java/org/apache/hadoop/hive/ql/exec/spark/session/SparkSessionManager.java PRE-CREATION
ql/src/java/org/apache/hadoop/hive/ql/exec/spark/session/SparkSessionManagerImpl.java PRE-CREATION
ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java fcfcf42
ql/src/test/org/apache/hadoop/hive/ql/exec/spark/session/TestSparkSessionManagerImpl.java PRE-CREATION
service/src/java/org/apache/hive/service/server/HiveServer2.java 0864dfb
Diff: https://reviews.apache.org/r/24763/diff/
Testing (updated)
-------
Added unittests to test SessionManagerImpl in single session mode (HiveCLI) and multi-session mode (HiveServer2). Also tested few queries from Hive CLI. Testing using actual HiveServer2 is blocked due to HIVE-7747.
Thanks,
Venki Korukanti