hive-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Lars Francke" <...@lars-francke.de>
Subject Re: Review Request 24293: HIVE-4629: HS2 should support an API to retrieve query logs
Date Tue, 05 Aug 2014 08:56:16 GMT

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


Thanks for taking care of this patch. I think the interface design looks good.

My comments are mostly (I think all but one or two) about code style. As there are so many
of them I can take the final patch and provided a cleaned one just prior to committing if
you want.


common/src/java/org/apache/hadoop/hive/conf/HiveConf.java
<https://reviews.apache.org/r/24293/#comment86725>

    line too long



service/if/TCLIService.thrift
<https://reviews.apache.org/r/24293/#comment86726>

    I know that no one else does it yet in this file and I haven't gotten around to finishing
my patch.
    
    But could you use this style of comments instead:
    
        /** Get the output result of a query. */
    
    Thank you! That will be automatically moved into a comment section (python, javadoc etc.)
by the Thrift compiler.



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

    You can just remove this whole comment. It's non Javadoc anyway so the @see doesn't make
sense and a proper one will be automatically generated.



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

    This line's now too long



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

    same as above about the comment



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

    remove comment



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

    Maybe it makes sense now to move the 1000 to a private/public static final



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

    Again I'd be in favor of removing this comment



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

    line's too long



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

    Remove comment or expand comment.
    
    If removed this class will pop up as undocumented which'd be correct as the current comment
doesn't help much :)



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

    may be final



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

    enums can be compared using ==



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

    I know the others have it here too but public abstract doesn't make sense in an interface



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

    typo "rr"



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

    may be static



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

    not needed



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

    super() and this. are not needed



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

    I don't understand how log data ends up in the writer? I looked for accesses of it but
it doesn't seem to be touched at all. What am I missing?
    
    Also for a little boost if the code stays like this you can move it after the null check
to avoid string conversion if the OperationLog is null



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

    unused



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

    long line



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

    The result of createNewFile should probably be checked. It may return false. So does delete()
but I think checking createNewFile is enough to cover your bases.



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

    toString is not needed



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

    Some javadoc would be nice on these three especially about behavior with exceptions and
when these are run (e.g. afterRun runs even when an exception is thrown in runInternal)



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

    no need to call toString explicitly



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

    unused



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

    remove or expand (the latter would be preferable here :) )



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

    unused



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

    can be final



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

    can be final



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

    unused



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

    unused



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

    this. not needed here and next line



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

    can be final and then renamed



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

    remove or document



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

    remove or document "empty" tags



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

    long line



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

    missing space before {



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

    missing space before {



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

    long line



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

    missing space before {



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

    long line



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

    missing space



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

    This seems more like a debug statement to me



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

    long line



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

    toString not needed (and then the line won't be too long either :) )



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

    new line missing



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

    long line



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

    toString not needed



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

    long line



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

    public modifier redundant for interfaces



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

    remove or document the @return tag



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

    long line



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

    toStrings not needed here



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

    long line



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

    enums can be compared using ==



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

    long line



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

    can be compared using ==



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

    long line



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

    long line
    
    maybe expand the message to say that it exists but is not a directory?



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

    long line



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

    long line



service/src/java/org/apache/hive/service/cli/thrift/ThriftCLIServiceClient.java
<https://reviews.apache.org/r/24293/#comment86787>

    long line but I'm in favor of removing it all together



service/src/java/org/apache/hive/service/cli/thrift/ThriftCLIServiceClient.java
<https://reviews.apache.org/r/24293/#comment86788>

    long line



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

    This Assert class should not be used anymore. Use the ones from org.junit.Assert instead
everywhere



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

    unused



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

    You're using JUnit3 style here but @Test annotations below from JUnit 4. I'd suggest removing
the TestCase extension and sticking to annotations



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

    missing space



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

    long line



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

    long line



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

    long line



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

    long line



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

    old Assert class (see above)



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

    long line



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

    long line



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

    long line
    toString not needed



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

    toString not needed



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

    old Assert class (see above)



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

    can be replaced with foreach



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

    can be replaced with foreach



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

    old Assert class (see above)


- Lars Francke


On Aug. 5, 2014, 3:47 a.m., Dong Chen wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24293/
> -----------------------------------------------------------
> 
> (Updated Aug. 5, 2014, 3:47 a.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-javabean/org/apache/hive/service/cli/thrift/TFetchType.java
PRE-CREATION 
>   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