Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/10855 )
Change subject: IMPALA-7213, IMPALA-7241: Port ReportExecStatus() RPC to use KRPC ...................................................................... Patch Set 14: (8 comments) http://gerrit.cloudera.org:8080/#/c/10855/13/be/src/runtime/coordinator-backend-state.h File be/src/runtime/coordinator-backend-state.h: http://gerrit.cloudera.org:8080/#/c/10855/13/be/src/runtime/coordinator-backend-state.h@152 PS13, Line 152: /// Updates 'this' with exec_status and the fragment intance's thrift prof > comment is out of date Done http://gerrit.cloudera.org:8080/#/c/10855/13/be/src/runtime/coordinator-backend-state.cc File be/src/runtime/coordinator-backend-state.cc: http://gerrit.cloudera.org:8080/#/c/10855/13/be/src/runtime/coordinator-backend-state.cc@523 PS13, Line 523: if (rows_counter != nullptr) { > line too long (91 > 90) Done http://gerrit.cloudera.org:8080/#/c/10855/13/be/src/runtime/coordinator-backend-state.cc@530 PS13, Line 530: } > line too long (91 > 90) Done http://gerrit.cloudera.org:8080/#/c/10855/13/be/src/service/control-service.cc File be/src/service/control-service.cc: http://gerrit.cloudera.org:8080/#/c/10855/13/be/src/service/control-service.cc@42 PS13, Line 42: QUEUE_LIMIT_MSG > Looks like a negative value here gives no limit? Maybe worth mentioning. Thanks for pointing that out. The handling of negative value is fixed in new PS. ParseUtil::ParseMemSpec() already distinguishes the input value between absolute value and percentage. If the input is percentage, it will compute the memory limit based on the reference value passed to ParseMemSpec(). http://gerrit.cloudera.org:8080/#/c/10855/13/be/src/service/control-service.cc@46 PS13, Line 46: if left at default value 0 > or negative Wording fixed. http://gerrit.cloudera.org:8080/#/c/10855/13/common/protobuf/control_service.proto File common/protobuf/control_service.proto: http://gerrit.cloudera.org:8080/#/c/10855/13/common/protobuf/control_service.proto@50 PS13, Line 50: optional KuduDmlStatsPB kudu_stats = 3; > is this, and elsewhere, referring to anything other than renaming, eg. to D Looking at (https://gerrit.cloudera.org/#/c/4985/1/common/thrift/ImpalaInternalService.thrift@430), it seems to suggest more than just renaming the struct. It also involves some sort of refactoring. That said, if we don't really know the meaning of this TODO statement, we can drop it too. What's your take ? http://gerrit.cloudera.org:8080/#/c/10855/13/common/protobuf/control_service.proto@107 PS13, Line 107: : FIRST_BATCH_PRODUCED > is this true if an error is encountered? Copied and pasted from Thrift Implementation. Fixed. http://gerrit.cloudera.org:8080/#/c/10855/13/common/protobuf/control_service.proto@154 PS13, Line 154: individual fra > I guess this and elsewhere was copied from the original thrift definition. I am not 100% sure about the historical context but I believe the rationale behind all the RPC versioning is that we can have backward compatibility between different versions of Impala (See also https://gerrit.cloudera.org/#/c/6535/4/common/thrift/ImpalaInternalService.thrift@698) That said, I don't think we ever implemented any true support for mixed versions of Impala (which is very useful for rolling upgrade). May be it's fair to drop the version string for now until we commit to supporting mixed version of Impala. In which case, we can take whatever snapshot of the structs then as V1. Before that, having the version string seems like some cruft that we don't need at the moment. Also, I don't quite understand the rationale behind the required fields in some of the existing Thrift structs as that seems to make it hard to change the definition of the struct later. So, for now, I am marking all fields to be optional (and latter version of protobuf actually gets rid of required/optional so everything is optional anyway). -- To view, visit http://gerrit.cloudera.org:8080/10855 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7638583b433dcac066b87198e448743d90415ebe Gerrit-Change-Number: 10855 Gerrit-PatchSet: 14 Gerrit-Owner: Michael Ho <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Michal Ostrowski <[email protected]> Gerrit-Reviewer: Thomas Marshall <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Mon, 08 Oct 2018 06:14:44 +0000 Gerrit-HasComments: Yes
