hive-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Thejas Nair" <the...@hortonworks.com>
Subject Re: Review Request 24293: HIVE-4629: HS2 should support an API to retrieve query logs
Date Wed, 20 Aug 2014 03:50:20 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24293/#review51047
-----------------------------------------------------------



service/src/java/org/apache/hive/service/cli/ICLIService.java
<https://reviews.apache.org/r/24293/#comment89010>

    Lets just keep one fetchResults method here.
    AFAIK, this is not a public API, so we can remove the older fetchResults methods.



service/src/java/org/apache/hive/service/cli/operation/GetCatalogsOperation.java
<https://reviews.apache.org/r/24293/#comment88952>

    The javadoc has a mismatch, it is referring to run method while the method name has changed.
    I don't think we need this javadoc.



service/src/java/org/apache/hive/service/cli/operation/Operation.java
<https://reviews.apache.org/r/24293/#comment88988>

    This set function is unused and looks unncessary.



service/src/java/org/apache/hive/service/cli/operation/Operation.java
<https://reviews.apache.org/r/24293/#comment88987>

    I don't see when this is likely to happen. Lets log a warning if this happens.



service/src/java/org/apache/hive/service/cli/operation/OperationLog.java
<https://reviews.apache.org/r/24293/#comment88962>

    should we warn here instead ?



service/src/java/org/apache/hive/service/cli/operation/OperationLog.java
<https://reviews.apache.org/r/24293/#comment89003>

    Looks like there can be race conditions where operation is just closed, and there is a
request to read results.
    I think it will be useful to have a isRemoved boolean, and check if that is true before
throwing the exception. If its true throw an exception saying that operation log has been
closed.



service/src/java/org/apache/hive/service/cli/operation/OperationLog.java
<https://reviews.apache.org/r/24293/#comment89004>

    same here, it would be good to check if the operation failed because of the log file being
removed.



service/src/java/org/apache/hive/service/cli/operation/OperationManager.java
<https://reviews.apache.org/r/24293/#comment88975>

    is this method needed ?



service/src/java/org/apache/hive/service/cli/operation/OperationManager.java
<https://reviews.apache.org/r/24293/#comment88976>

    Since this is log specific schema, how about changing the name to getLogSchema



service/src/java/org/apache/hive/service/cli/operation/SQLOperation.java
<https://reviews.apache.org/r/24293/#comment88995>

    I think it is better to not fail because logging could not be done. Instead log a warning
or error message and set isOperationLogEnabled=false, like you are doing in other places.



service/src/java/org/apache/hive/service/cli/operation/SQLOperation.java
<https://reviews.apache.org/r/24293/#comment88996>

    doesn't this need to be done for other Operation sub classes ?



service/src/java/org/apache/hive/service/cli/session/HiveSession.java
<https://reviews.apache.org/r/24293/#comment89006>

    Lets remove this function, seems unused, and it is redundant.



service/src/java/org/apache/hive/service/cli/session/HiveSessionImpl.java
<https://reviews.apache.org/r/24293/#comment88997>

    Lets not fail the connection close because of this. Instead log an error.



service/src/java/org/apache/hive/service/cli/session/SessionManager.java
<https://reviews.apache.org/r/24293/#comment88998>

    lets call initLogRootDir before addService(operationManager), as operationmanager.init
does log initialization.



service/src/java/org/apache/hive/service/cli/session/SessionManager.java
<https://reviews.apache.org/r/24293/#comment88999>

    Please change to "Failed to schedule cleanup HS2 opereration logging root dir: "



service/src/java/org/apache/hive/service/cli/session/SessionManager.java
<https://reviews.apache.org/r/24293/#comment89000>

    Lets print the dir path here as well.



service/src/test/org/apache/hive/service/cli/operation/TestOperationLoggingAPI.java
<https://reviews.apache.org/r/24293/#comment89007>

    Thanks for the good set of tests!



service/src/test/org/apache/hive/service/cli/operation/TestOperationLoggingAPI.java
<https://reviews.apache.org/r/24293/#comment89009>

    Lets also check the error message in the exception here.


- Thejas Nair


On Aug. 14, 2014, 3:09 p.m., Dong Chen wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24293/
> -----------------------------------------------------------
> 
> (Updated Aug. 14, 2014, 3:09 p.m.)
> 
> 
> Review request for hive.
> 
> 
> Repository: hive-git
> 
> 
> Description
> -------
> 
> HIVE-4629: HS2 should support an API to retrieve query logs
> HiveServer2 should support an API to retrieve query logs. This is particularly relevant
because HiveServer2 supports async execution but doesn't provide a way to report progress.
Providing an API to retrieve query logs will help report progress to the client.
> 
> 
> Diffs
> -----
> 
>   common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 3bfc681 
>   service/if/TCLIService.thrift 80086b4 
>   service/src/gen/thrift/gen-cpp/TCLIService_types.h 1b37fb5 
>   service/src/gen/thrift/gen-cpp/TCLIService_types.cpp d5f98a8 
>   service/src/gen/thrift/gen-javabean/org/apache/hive/service/cli/thrift/TFetchResultsReq.java
808b73f 
>   service/src/gen/thrift/gen-py/TCLIService/ttypes.py 2cbbdd8 
>   service/src/gen/thrift/gen-rb/t_c_l_i_service_types.rb 93f9a81 
>   service/src/java/org/apache/hive/service/cli/CLIService.java add37a1 
>   service/src/java/org/apache/hive/service/cli/CLIServiceClient.java 87c10b9 
>   service/src/java/org/apache/hive/service/cli/EmbeddedCLIServiceClient.java f665146

>   service/src/java/org/apache/hive/service/cli/FetchType.java PRE-CREATION 
>   service/src/java/org/apache/hive/service/cli/ICLIService.java c569796 
>   service/src/java/org/apache/hive/service/cli/operation/GetCatalogsOperation.java c9fd5f9

>   service/src/java/org/apache/hive/service/cli/operation/GetColumnsOperation.java caf413d

>   service/src/java/org/apache/hive/service/cli/operation/GetFunctionsOperation.java fd4e94d

>   service/src/java/org/apache/hive/service/cli/operation/GetSchemasOperation.java ebca996

>   service/src/java/org/apache/hive/service/cli/operation/GetTableTypesOperation.java
05991e0 
>   service/src/java/org/apache/hive/service/cli/operation/GetTablesOperation.java 315dbea

>   service/src/java/org/apache/hive/service/cli/operation/GetTypeInfoOperation.java 0ec2543

>   service/src/java/org/apache/hive/service/cli/operation/HiveCommandOperation.java 3d3fddc

>   service/src/java/org/apache/hive/service/cli/operation/LogDivertAppender.java PRE-CREATION

>   service/src/java/org/apache/hive/service/cli/operation/MetadataOperation.java e0d17a1

>   service/src/java/org/apache/hive/service/cli/operation/Operation.java 45fbd61 
>   service/src/java/org/apache/hive/service/cli/operation/OperationLog.java PRE-CREATION

>   service/src/java/org/apache/hive/service/cli/operation/OperationManager.java 21c33bc

>   service/src/java/org/apache/hive/service/cli/operation/SQLOperation.java de54ca1 
>   service/src/java/org/apache/hive/service/cli/session/HiveSession.java 9785e95 
>   service/src/java/org/apache/hive/service/cli/session/HiveSessionBase.java 4c3164e 
>   service/src/java/org/apache/hive/service/cli/session/HiveSessionImpl.java b39d64d 
>   service/src/java/org/apache/hive/service/cli/session/SessionManager.java 816bea4 
>   service/src/java/org/apache/hive/service/cli/thrift/ThriftCLIService.java 5c87bcb 
>   service/src/java/org/apache/hive/service/cli/thrift/ThriftCLIServiceClient.java e3384d3

>   service/src/test/org/apache/hive/service/cli/operation/TestOperationLoggingAPI.java
PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/24293/diff/
> 
> 
> Testing
> -------
> 
> UT passed.
> 
> 
> Thanks,
> 
> Dong Chen
> 
>


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