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