[Impala-ASF-CR] IMPALA-7213, IMPALA-7241: Port ReportExecStatus() RPC to use KRPC

["Thomas Marshall (Code Review)" <ge...@cloudera.org>]

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 <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Michal Ostrowski <mo...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 02 Oct 2018 21:04:11 +0000
Gerrit-HasComments: Yes