Thomas Marshall 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 13: (6 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, the fragment instances' TExecStats in comment is out of date 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. Also looks like MEM_UNITS_HELP_MSG says that this can be a '%', but then you ignore 'is_percent' below so eg. '50%' would be interpreted as 50 bytes. 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 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: // TODO: Refactor to reflect usage by other DML statements. is this, and elsewhere, referring to anything other than renaming, eg. to DMLStatsPB? If not, is it worth just resolving this now while you're here? http://gerrit.cloudera.org:8080/#/c/10855/13/common/protobuf/control_service.proto@107 PS13, Line 107: one state will only strictly be reached after all : // the previous states is this true if an error is encountered? http://gerrit.cloudera.org:8080/#/c/10855/13/common/protobuf/control_service.proto@154 PS13, Line 154: required in V1 I guess this and elsewhere was copied from the original thrift definition. Do they still make sense, given that this is V1 of the protobuf implementation? -- 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: 13 Gerrit-Owner: Michael Ho <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Michal Ostrowski <[email protected]> Gerrit-Reviewer: Sailesh Mukil <[email protected]> Gerrit-Reviewer: Thomas Marshall <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Tue, 02 Oct 2018 21:04:11 +0000 Gerrit-HasComments: Yes
