----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/14326/#review27210 -----------------------------------------------------------
common/src/java/org/apache/hadoop/hive/conf/HiveConf.java <https://reviews.apache.org/r/14326/#comment52909> Is this in memory logging for the server process, sessions, operations, or ...? Please make the property name more specific, and please change the ordering of the subnames so that they run from general to specific, e.g. hive.server2.logging.operation.* common/src/java/org/apache/hadoop/hive/conf/HiveConf.java <https://reviews.apache.org/r/14326/#comment52910> Please use kilobytes instead of bytes. conf/hive-default.xml.template <https://reviews.apache.org/r/14326/#comment52911> It's an operation log, not a query log. Also, what are the units of this value? jdbc/src/test/org/apache/hive/jdbc/TestJdbcDriver2.java <https://reviews.apache.org/r/14326/#comment52928> This feature needs to be tested at the Thrift level. Please include a test where getOperationLog is called more than once for the same operation, and a test where getOperationLog is demonstrated with asynchronous queries. jdbc/src/test/org/apache/hive/jdbc/TestJdbcDriver2.java <https://reviews.apache.org/r/14326/#comment52927> This looks like a substring from the previous check. service/if/TCLIService.thrift <https://reviews.apache.org/r/14326/#comment52912> Please change the name to GetOperationLog() service/if/TCLIService.thrift <https://reviews.apache.org/r/14326/#comment52914> Does this return one line from the log per call, or do I get the whole 128kb buffer? What happens if I call this RPC multiple times on the same operation handle? What happens if the circular buffer rolls over between two calls? service/if/TCLIService.thrift <https://reviews.apache.org/r/14326/#comment52913> Formatting service/src/java/org/apache/hive/service/cli/CLIService.java <https://reviews.apache.org/r/14326/#comment52915> Why isn't OperationLog a member of the o.a.h.service.cli.operation package? service/src/java/org/apache/hive/service/cli/log/LinkedStringBuffer.java <https://reviews.apache.org/r/14326/#comment52917> The name of this class should describe the functionality as opposed to the implementation. service/src/java/org/apache/hive/service/cli/log/LinkedStringBuffer.java <https://reviews.apache.org/r/14326/#comment52916> StringBuilder provides a delete(start, end) method which can be used to easily implement a ring buffer. I think that would be a better option here since it would eliminate the need to iterate over every log entry in read(). service/src/java/org/apache/hive/service/cli/log/LogManager.java <https://reviews.apache.org/r/14326/#comment52919> I think most of this code belongs in OperationManager as opposed to its own class. service/src/java/org/apache/hive/service/cli/log/LogManager.java <https://reviews.apache.org/r/14326/#comment52918> Thread names are mutable and don't have to be unique. I think you want to use threadId instead. service/src/java/org/apache/hive/service/cli/log/LogManager.java <https://reviews.apache.org/r/14326/#comment52920> spelling service/src/java/org/apache/hive/service/cli/log/OperationLog.java <https://reviews.apache.org/r/14326/#comment52921> Please move this to o.a.h.service.cli.operation.* service/src/java/org/apache/hive/service/cli/log/OperationLog.java <https://reviews.apache.org/r/14326/#comment52922> Does this comment imply that when operation logging is enabled log messages generated by operations won't appear in the system logs? Also, can you please include a paragraph somewhere that provides a high-level overview of the operation logging mechanism? What expectations does it make about the relationship between Sessions/Operations and threads, and does it support asynchronous execution? service/src/java/org/apache/hive/service/cli/operation/OperationManager.java <https://reviews.apache.org/r/14326/#comment52923> Seems like most of the operation logging code should go in this class. service/src/java/org/apache/hive/service/cli/session/HiveSession.java <https://reviews.apache.org/r/14326/#comment52924> I don't understand why the OperationLogger hangs off the SessionManager instead of the OperationManager. service/src/java/org/apache/hive/service/cli/session/HiveSessionImpl.java <https://reviews.apache.org/r/14326/#comment52925> If the established protocol for registering a thread is to always unregister it first, why not have registerCurrentThread call unregisterCurrentThread? service/src/java/org/apache/hive/service/cli/session/HiveSessionImpl.java <https://reviews.apache.org/r/14326/#comment52926> Does operation logging work for executeStatementAsync()? - Carl Steinbach 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 > >
