You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hive.apache.org by Brock Noland <br...@cloudera.com> on 2013/10/04 20:47:46 UTC
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/#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
> 
>