hive-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Carl Steinbach" <c...@cloudera.com>
Subject Re: Review Request 14326: HIVE-4629. HS2 should support an API to retrieve query logs
Date Fri, 18 Oct 2013 23:50:46 GMT

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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message