----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38783/#review101167 -----------------------------------------------------------
Mostly minor comments on the patch itself. Some more suggestions: - Is a configuration flag required to go back to the original behaviour in case of issues - i.e. defaults to async via the CLI, but an option exists to go back to the current model - where everything happens in the same state. I'm a little wary of ThreadLocals used in various places causing issues. - Change Q tests to use this mechanism as well. They open their own session if I'm not mistaken. - In getSesssion() - if the future has not completed, does it make sense to log a line every 10 seconds or so to indicate that a query is not running due to inadequate cluster resources / waiting for the AM. - Would statements like show databases; / describe table hit this code path. I don't think getSession would be invoked (that's only from TezTask when a DAG is created) ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionState.java (line 151) <https://reviews.apache.org/r/38783/#comment158511> Minor: Second parameter needed? Is always null ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionState.java (line 274) <https://reviews.apache.org/r/38783/#comment158512> Typo: initilize ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionState.java (line 353) <https://reviews.apache.org/r/38783/#comment158513> Does sessionFuture need to be set to null as well. Looks like a TezSessionState object can be re-used with different AMs / Applications. ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionState.java (line 361) <https://reviews.apache.org/r/38783/#comment158514> Minor: parameter name can cause confusion. Isn't always the asyncSession ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionState.java (line 380) <https://reviews.apache.org/r/38783/#comment158517> I believe the behaviour will be similar to what it is rightnow for invocation of open(). i.e. a RuntimeException will end up being thrown in case of most errors. Wondering if a SessionNotRunning exception from waitTillReady() needs to be handled differently in the async model. Separate jira anyway. - Siddharth Seth On Sept. 26, 2015, 1:36 a.m., Sergey Shelukhin wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/38783/ > ----------------------------------------------------------- > > (Updated Sept. 26, 2015, 1:36 a.m.) > > > Review request for hive, Gunther Hagleitner and Siddharth Seth. > > > Repository: hive-git > > > Description > ------- > > See JIRA > > > Diffs > ----- > > cli/src/java/org/apache/hadoop/hive/cli/CliDriver.java d62fd5c > ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionState.java 568ebbe > ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java 014941e > > Diff: https://reviews.apache.org/r/38783/diff/ > > > Testing > ------- > > > Thanks, > > Sergey Shelukhin > >
