Re: Review Request 24293: 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/24293/ --- (Updated Aug. 26, 2014, 8:24 a.m.) Review request for hive. Changes --- Update patch V8 to address the comments from Thejas and Carl. Also rebase it from latest trunk. 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 (updated) - common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 7f4afd9 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 d2cdfc1 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 eee1cc6 service/src/java/org/apache/hive/service/cli/session/HiveSessionImpl.java bc0a02c service/src/java/org/apache/hive/service/cli/session/SessionManager.java d573592 service/src/java/org/apache/hive/service/cli/thrift/ThriftCLIService.java be2eb01 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
Re: Review Request 24293: HIVE-4629: HS2 should support an API to retrieve query logs
On Aug. 20, 2014, 3:50 a.m., Thejas Nair wrote: service/src/java/org/apache/hive/service/cli/operation/SQLOperation.java, line 284 https://reviews.apache.org/r/24293/diff/4/?file=660556#file660556line284 doesn't this need to be done for other Operation sub classes ? Thanks for your review. You are right. I think other Operation sub classes also need this. The method cleanupOperationLog() is also invoked by close() method of class HiveCommandOpeation and class MetadataOperation. This ensure all kinds of Operations clean up their logs after being closed. - Dong --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24293/#review51047 --- On Aug. 26, 2014, 8:24 a.m., Dong Chen wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24293/ --- (Updated Aug. 26, 2014, 8:24 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 7f4afd9 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 d2cdfc1 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 eee1cc6 service/src/java/org/apache/hive/service/cli/session/HiveSessionImpl.java bc0a02c service/src/java/org/apache/hive/service/cli/session/SessionManager.java d573592 service/src/java/org/apache/hive/service/cli/thrift/ThriftCLIService.java be2eb01 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
Re: Review Request 24293: 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/24293/#review51230 --- service/if/TCLIService.thrift https://reviews.apache.org/r/24293/#comment89316 Please use // for comments (like the rest of the file). - Carl Steinbach 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
Re: Review Request 24293: HIVE-4629: HS2 should support an API to retrieve query logs
On Aug. 5, 2014, 8:56 a.m., Lars Francke wrote: service/if/TCLIService.thrift, line 1043 https://reviews.apache.org/r/24293/diff/1/?file=651542#file651542line1043 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. Dong Chen wrote: Thanks for you reminding. This comment style makes the generated code look better. Not sure whether you are working on changing all the comment style in TCLIService.thrift file. So I just change the 3 comments related with this fix. If not, I'm glad to make the changes of all the comments in the thrift through this patch or another new Jira. This sounds like a nice feature to exploit, but I think it should be saved for a separate patch that updates all of the comments at once. - Carl --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24293/#review49573 --- 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
Re: Review Request 24293: 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/24293/#review50977 --- Hi, This patch looks really good! I was not clear when I said how we should define the new fetchResults method. I hope my response below is clear, if not, please let me know! service/src/java/org/apache/hive/service/cli/CLIServiceClient.java https://reviews.apache.org/r/24293/#comment88863 Thank you very much for removing the thrift enum! That resolves the thrift enum compatability issue! I should have been a more clear on the other issue I was describing. I have felt for some time we should change the way we do RPC in Hive. Today we define specific methods for the use case at hand. This causes method explosion. For example after this patch we would have three method signatures which fetch results. Going forward I think we should define methods differently. For example, for this method I think we should define the classes: FetchResultsRequest and FetchResultsResponse and then have a new method: FetchResultsResponse fetchResults(FetchResultsRequest request) throws HiveSQLException and then all of the arguments can be defined inside FetchResultsRequest. That way everytime we add an argument, we don't to define a new public RPC method. I have described this approach on this mail here: http://mail-archives.apache.org/mod_mbox/hive-dev/201403.mbox/%3CCAFukC=6xss1kjgad7hv2v4wwoigjzctm1rujcczsocdj8x3...@mail.gmail.com%3E - Brock Noland 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
Re: Review Request 24293: 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/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 -
Re: Review Request 24293: 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/24293/#review51071 --- service/src/test/org/apache/hive/service/cli/operation/TestOperationLoggingAPI.java https://reviews.apache.org/r/24293/#comment89028 Can you also try fetching the log here, just to make sure that no error is thrown. Then verify that the accumulated log has expected results. You can also reduce the sleep from 1000 to 100 or so, so that multiple fetch results on log get called. - 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
Re: Review Request 24293: HIVE-4629: HS2 should support an API to retrieve query logs
On Aug. 12, 2014, 4:54 p.m., Brock Noland wrote: Hi Dong! Excellent work! I have given it a first review and I think it looks great. I have some initial feedback below. Hi Brock, thanks for your review and the comments are very useful. I update the patch based on them and leave some uncertain thought about the backwards compatibility below. On Aug. 12, 2014, 4:54 p.m., Brock Noland wrote: service/if/TCLIService.thrift, line 1042 https://reviews.apache.org/r/24293/diff/3/?file=654749#file654749line1042 I wonder if we should use Integer flags as opposed to enums as we've found Thrift Enums to be backwards incompatible in the past: https://issues.apache.org/jira/browse/HIVE-6050 In order to use Integer flags as opposed to enum, I remove the TFetchType enum and change the type of field “fetchType” of TFetchResultsReq to i16 (short). And FetchType class methods are changed for converting the short type to FetchType for client and service layer interface usage. A question is: it works, but using number 0 and 1 instead of enum value QUERY_OUTPUT and LOG might make the Thrift interface not easy to understand. On Aug. 12, 2014, 4:54 p.m., Brock Noland wrote: service/src/java/org/apache/hive/service/cli/ICLIService.java, line 89 https://reviews.apache.org/r/24293/diff/3/?file=654760#file654760line89 Thrift is also not backwards compatible when the number of arguments changes. I think we should define a new fetchResults method which takes a single argument FetchResultsRequest and returns a single response FetchResultsResponse. This new fetchResults method should cover both fetchResults methods use cases todays. We can then deprecate to other two methods. Making the fetchResults method backwards compatible is a good suggestion. I’m a little uncertain whether understanding the problem correctly and below is some thought. I think Thrift layer might be backwards compatible, since the method FetchResults takes a single argument TFetchResultsReq and returns a single response TFetchResultsResp. We just add an optional short type field fetchType into TFetchResultsReq and it may not impact compatibility. For client and service layer interface (ICLIService), defining a new fetchResults method taking a single argument (which wrapping OperationHandle, Orientation, etc) is a good way. But it might not be consistent, since other methods always explicitly take OperationHandle as an argument. How about adding 2 new overloaded fetchResults methods, each with one extra argument fetchType and deprecate the original two methods? (code in the updated patch for reference). This way may also make client side and service side implementation a little simple. - Dong --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24293/#review50335 --- On Aug. 7, 2014, 5:37 p.m., Dong Chen wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24293/ --- (Updated Aug. 7, 2014, 5:37 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-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
Re: Review Request 24293: 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/24293/ --- (Updated Aug. 14, 2014, 3:09 p.m.) Review request for hive. Changes --- Update patch for backwards compatibility. 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 (updated) - 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
Re: Review Request 24293: 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/24293/#review50335 --- Hi Dong! Excellent work! I have given it a first review and I think it looks great. I have some initial feedback below. service/if/TCLIService.thrift https://reviews.apache.org/r/24293/#comment88011 I wonder if we should use Integer flags as opposed to enums as we've found Thrift Enums to be backwards incompatible in the past: https://issues.apache.org/jira/browse/HIVE-6050 service/src/java/org/apache/hive/service/cli/ICLIService.java https://reviews.apache.org/r/24293/#comment88012 Thrift is also not backwards compatible when the number of arguments changes. I think we should define a new fetchResults method which takes a single argument FetchResultsRequest and returns a single response FetchResultsResponse. This new fetchResults method should cover both fetchResults methods use cases todays. We can then deprecate to other two methods. service/src/java/org/apache/hive/service/cli/operation/Operation.java https://reviews.apache.org/r/24293/#comment88015 Should be be logged as ERROR? service/src/java/org/apache/hive/service/cli/operation/Operation.java https://reviews.apache.org/r/24293/#comment88014 I think the operationLog.close() method should be in an else block to avoid an NPE. service/src/java/org/apache/hive/service/cli/operation/OperationLog.java https://reviews.apache.org/r/24293/#comment88016 private to be consistent service/src/java/org/apache/hive/service/cli/operation/OperationLog.java https://reviews.apache.org/r/24293/#comment88017 Assuming the exception will be logged by the caller (let's verify that) we should remove this log statement. service/src/java/org/apache/hive/service/cli/operation/OperationLog.java https://reviews.apache.org/r/24293/#comment88018 This msg should be passed into HiveSQLException. Additionally assuming this will be logged by the caller (let's verify that) , we should remove the log stmt. service/src/java/org/apache/hive/service/cli/operation/SQLOperation.java https://reviews.apache.org/r/24293/#comment88020 the name runInternal2 is not descriptive. Let's find a better name? service/src/java/org/apache/hive/service/cli/session/HiveSessionImpl.java https://reviews.apache.org/r/24293/#comment88025 if the caller is going to log this exception (let's verify that) then we should not log this message. - Brock Noland On Aug. 7, 2014, 5:37 p.m., Dong Chen wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24293/ --- (Updated Aug. 7, 2014, 5:37 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-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
Re: Review Request 24293: HIVE-4629: HS2 should support an API to retrieve query logs
On Aug. 5, 2014, 8:56 a.m., Lars Francke wrote: service/if/TCLIService.thrift, line 1043 https://reviews.apache.org/r/24293/diff/1/?file=651542#file651542line1043 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. Thanks for you reminding. This comment style makes the generated code look better. Not sure whether you are working on changing all the comment style in TCLIService.thrift file. So I just change the 3 comments related with this fix. If not, I'm glad to make the changes of all the comments in the thrift through this patch or another new Jira. - Dong --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24293/#review49573 --- 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
Re: Review Request 24293: HIVE-4629: HS2 should support an API to retrieve query logs
On Aug. 5, 2014, 8:56 a.m., Lars Francke wrote: service/src/java/org/apache/hive/service/cli/operation/LogDivertAppender.java, line 81 https://reviews.apache.org/r/24293/diff/1/?file=651562#file651562line81 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 This LogDivertAppender inherits from WriterAppender, and when its method subAppend(event) is invoked, the first line super.subAppend(event) will write the log into writer. Not matter the OperationLog is null or not, the writer should be reset, since the log in it will be not used any more in this Appender. Otherwise, the remaining log in writer might mix with next log. So maybe we could keep the access and null check order. :) - Dong --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24293/#review49573 --- 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
Re: Review Request 24293: HIVE-4629: HS2 should support an API to retrieve query logs
On Aug. 5, 2014, 8:56 a.m., Lars Francke wrote: service/src/java/org/apache/hive/service/cli/operation/OperationLog.java, line 58 https://reviews.apache.org/r/24293/diff/1/?file=651565#file651565line58 can be final and then renamed Thank you! I made it final and it is a good point. But a little confused about the renamed? Do you mean the variable name threadLocalOperationLog? - Dong --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24293/#review49573 --- 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
Re: Review Request 24293: 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/24293/#review49911 --- service/if/TCLIService.thrift https://reviews.apache.org/r/24293/#comment87340 I have a partial patch that changes all of them and I planned on submitting it when I'm back from holiday. - 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
Re: Review Request 24293: HIVE-4629: HS2 should support an API to retrieve query logs
On Aug. 5, 2014, 8:56 a.m., Lars Francke wrote: service/src/java/org/apache/hive/service/cli/operation/OperationLog.java, line 58 https://reviews.apache.org/r/24293/diff/1/?file=651565#file651565line58 can be final and then renamed Dong Chen wrote: Thank you! I made it final and it is a good point. But a little confused about the renamed? Do you mean the variable name threadLocalOperationLog? static finals have the naming convention of being all upper case with underscores in between. So it should be THREAD_LOCAL_OPERATION_LOG On Aug. 5, 2014, 8:56 a.m., Lars Francke wrote: service/src/java/org/apache/hive/service/cli/operation/LogDivertAppender.java, line 81 https://reviews.apache.org/r/24293/diff/1/?file=651562#file651562line81 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 Dong Chen wrote: This LogDivertAppender inherits from WriterAppender, and when its method subAppend(event) is invoked, the first line super.subAppend(event) will write the log into writer. Not matter the OperationLog is null or not, the writer should be reset, since the log in it will be not used any more in this Appender. Otherwise, the remaining log in writer might mix with next log. So maybe we could keep the access and null check order. :) Ahh thanks for the explanation. I missed the setWriter bit. - Lars --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24293/#review49573 --- 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
Re: Review Request 24293: HIVE-4629: HS2 should support an API to retrieve query logs
On Aug. 7, 2014, 4:57 p.m., Lars Francke wrote: service/if/TCLIService.thrift, line 1043 https://reviews.apache.org/r/24293/diff/1/?file=651542#file651542line1043 I have a partial patch that changes all of them and I planned on submitting it when I'm back from holiday. Sorry I messed up RB. This was meant as a reply to your Thrift comment answer and not a new issue. - Lars --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24293/#review49911 --- 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
Re: Review Request 24293: 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/24293/ --- (Updated Aug. 7, 2014, 5:06 p.m.) Review request for hive. Changes --- Updated patch HIVE_4629.5.patch. 1. address the review comments. 2. fix the failed case in HIVE QA. (org.apache.hive.service.cli.TestEmbeddedThriftBinaryCLIService.testExecuteStatementAsync) 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 (updated) - 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
Re: Review Request 24293: HIVE-4629: HS2 should support an API to retrieve query logs
On Aug. 7, 2014, 4:57 p.m., Lars Francke wrote: service/if/TCLIService.thrift, line 1043 https://reviews.apache.org/r/24293/diff/1/?file=651542#file651542line1043 I have a partial patch that changes all of them and I planned on submitting it when I'm back from holiday. Lars Francke wrote: Sorry I messed up RB. This was meant as a reply to your Thrift comment answer and not a new issue. That's OK. :) Got it. Thanks. So I will just change the 3 Thrift comments related with this patch. - Dong --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24293/#review49911 --- On Aug. 7, 2014, 5:06 p.m., Dong Chen wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24293/ --- (Updated Aug. 7, 2014, 5:06 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-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
Re: Review Request 24293: 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/24293/ --- (Updated Aug. 7, 2014, 5:37 p.m.) Review request for hive. Changes --- A little change: rename the static final variable threadLocalOperationLog 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 (updated) - 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
Re: Review Request 24293: HIVE-4629: HS2 should support an API to retrieve query logs
On Aug. 5, 2014, 8:56 a.m., Lars Francke wrote: service/src/java/org/apache/hive/service/cli/operation/OperationLog.java, line 58 https://reviews.apache.org/r/24293/diff/1/?file=651565#file651565line58 can be final and then renamed Dong Chen wrote: Thank you! I made it final and it is a good point. But a little confused about the renamed? Do you mean the variable name threadLocalOperationLog? Lars Francke wrote: static finals have the naming convention of being all upper case with underscores in between. So it should be THREAD_LOCAL_OPERATION_LOG Oh, right! I should keep this in mind. :) - Dong --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24293/#review49573 --- On Aug. 7, 2014, 5:37 p.m., Dong Chen wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24293/ --- (Updated Aug. 7, 2014, 5:37 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-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
Re: Review Request 24293: 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/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
Review Request 24293: 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/24293/ --- 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
Re: Review Request 24293: 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/24293/#review49563 --- This is awesome! I have two minor comments and I will let others do a more thorough review. service/src/java/org/apache/hive/service/cli/operation/Operation.java https://reviews.apache.org/r/24293/#comment86709 let's log the exception here service/src/java/org/apache/hive/service/cli/operation/Operation.java https://reviews.apache.org/r/24293/#comment86710 Let's log the exception here as well - Brock Noland 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
