----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/14326/#review26683 -----------------------------------------------------------
Shreepadma, This looks like a very useful feature for clients of HS2! Indeed I wish all databases had this feature. The patch looks very good. I have noted a few items below, nothing major, basically a bunch of nits. Brock jdbc/src/test/org/apache/hive/jdbc/TestJdbcDriver2.java <https://reviews.apache.org/r/14326/#comment51991> Let's add a test where we call getLog without any query. jdbc/src/test/org/apache/hive/jdbc/TestJdbcDriver2.java <https://reviews.apache.org/r/14326/#comment51987> Why even catch this if we rethrow immediately. jdbc/src/test/org/apache/hive/jdbc/TestJdbcDriver2.java <https://reviews.apache.org/r/14326/#comment51988> I think we should append these two error messages with log. service/if/TCLIService.thrift <https://reviews.apache.org/r/14326/#comment51976> Let's trim this trailing ws. (understand it's not yours.) service/src/java/org/apache/hive/service/cli/CLIService.java <https://reviews.apache.org/r/14326/#comment51977> Should this be called in a finally block? Same question for the items below. service/src/java/org/apache/hive/service/cli/CLIService.java <https://reviews.apache.org/r/14326/#comment51986> Is this useful at the INFO level? It's completely your call but I was just curious. service/src/java/org/apache/hive/service/cli/log/LinkedStringBuffer.java <https://reviews.apache.org/r/14326/#comment51984> Looks like we only require the List interface on the LHS? service/src/java/org/apache/hive/service/cli/log/LinkedStringBuffer.java <https://reviews.apache.org/r/14326/#comment51985> Internally we storing this as an int. Should the return type be a int or the internal type be a long? service/src/java/org/apache/hive/service/cli/log/LogDivertAppender.java <https://reviews.apache.org/r/14326/#comment51973> When we drop an event, that means it's not stored in memory. Is it still logged to the HS2 log file? Since we have a limit on the amount of data we store per log capture, I guess the question is, is all this data also sent to the HS2 log file? service/src/java/org/apache/hive/service/cli/log/LogManager.java <https://reviews.apache.org/r/14326/#comment51983> The order of members for this class should be: static final final non-final service/src/java/org/apache/hive/service/cli/log/LogManager.java <https://reviews.apache.org/r/14326/#comment51981> These two should start with a lower case cahr service/src/java/org/apache/hive/service/cli/log/LogManager.java <https://reviews.apache.org/r/14326/#comment51982> LOG should be final service/src/java/org/apache/hive/service/cli/log/LogManager.java <https://reviews.apache.org/r/14326/#comment51980> The variable should start with a lowercase char. Same same below and above. service/src/java/org/apache/hive/service/cli/log/LogManager.java <https://reviews.apache.org/r/14326/#comment51969> spelling service/src/java/org/apache/hive/service/cli/log/LogManager.java <https://reviews.apache.org/r/14326/#comment51989> Slightly confusing. I would expect: if(operationLog == nul) { if(createIfAbsent) { doCreate(); } else { throw } } service/src/java/org/apache/hive/service/cli/log/LogManager.java <https://reviews.apache.org/r/14326/#comment51990> Operation is spelled wrong service/src/java/org/apache/hive/service/cli/session/HiveSession.java <https://reviews.apache.org/r/14326/#comment51970> trailing ws service/src/java/org/apache/hive/service/cli/session/HiveSessionImpl.java <https://reviews.apache.org/r/14326/#comment51979> Similar to above, should this be part of the finally? service/src/java/org/apache/hive/service/cli/thrift/ThriftCLIService.java <https://reviews.apache.org/r/14326/#comment51971> I know we are printing stack traces to syserr elsewhere in this file but lets' use the logger? service/src/java/org/apache/hive/service/cli/thrift/ThriftCLIServiceClient.java <https://reviews.apache.org/r/14326/#comment51972> What encoding should this be, UTF-8? - Brock Noland On Sept. 25, 2013, 12:08 a.m., Shreepadma Venugopalan wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/14326/ > ----------------------------------------------------------- > > (Updated Sept. 25, 2013, 12:08 a.m.) > > > Review request for hive and Brock Noland. > > > Bugs: HIVE-4629 > https://issues.apache.org/jira/browse/HIVE-4629 > > > Repository: hive-git > > > Description > ------- > > Adds a new API to HS2, String getLog(OperationHandle opHandle) that returns > the query log for a given operation handle. The log is maintained in memory > as a circular buffer. The default size is 128 KB, but can be configured by > the user. Logging is initialized if hive.server2.in.mem.logging is set to > true. > > Log object is created in > executeStatement,getColumns,getTables,getSchemas,getCatalogs,getTypeInfo,getFunctions > and destroyed in closeOperation, cancelOperation. > > > Diffs > ----- > > common/src/java/org/apache/hadoop/hive/conf/HiveConf.java e971644 > conf/hive-default.xml.template 1ee756c > jdbc/src/java/org/apache/hive/jdbc/HiveStatement.java 2912ece > jdbc/src/test/org/apache/hive/jdbc/TestJdbcDriver2.java 09ab3c2 > service/if/TCLIService.thrift 6e20375 > service/src/java/org/apache/hive/service/cli/CLIService.java 1a7f338 > service/src/java/org/apache/hive/service/cli/CLIServiceClient.java 14ef54f > service/src/java/org/apache/hive/service/cli/EmbeddedCLIServiceClient.java > 9dca874 > service/src/java/org/apache/hive/service/cli/ICLIService.java f647ce6 > service/src/java/org/apache/hive/service/cli/log/LinkedStringBuffer.java > PRE-CREATION > service/src/java/org/apache/hive/service/cli/log/LogDivertAppender.java > PRE-CREATION > service/src/java/org/apache/hive/service/cli/log/LogManager.java > PRE-CREATION > service/src/java/org/apache/hive/service/cli/log/OperationLog.java > PRE-CREATION > > service/src/java/org/apache/hive/service/cli/operation/OperationManager.java > 1f78a1d > service/src/java/org/apache/hive/service/cli/session/HiveSession.java > 00058cc > service/src/java/org/apache/hive/service/cli/session/HiveSessionImpl.java > 11c96b2 > service/src/java/org/apache/hive/service/cli/session/SessionManager.java > f392d62 > service/src/java/org/apache/hive/service/cli/thrift/ThriftCLIService.java > 2f2866f > > service/src/java/org/apache/hive/service/cli/thrift/ThriftCLIServiceClient.java > 9bb2a0f > > Diff: https://reviews.apache.org/r/14326/diff/ > > > Testing > ------- > > Add a new unit test to test log retrieval. > > > Thanks, > > Shreepadma Venugopalan > >
