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