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