Home | About | Sematext search-lucene.com search-hadoop.com
 Search Hadoop and all its subprojects:

Switch to Threaded View
Hive >> mail # dev >> Review Request 14326: HIVE-4629. HS2 should support an API to retrieve query logs


Copy link to this message
-
Re: Review Request 14326: HIVE-4629. HS2 should support an API to retrieve query logs

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