Joe McDonnell has posted comments on this change. Change subject: IMPALA-5892: Allow reporting status independent of fragment instance ......................................................................
Patch Set 1: (8 comments) http://gerrit.cloudera.org:8080/#/c/7943/1/be/src/runtime/coordinator-backend-state.cc File be/src/runtime/coordinator-backend-state.cc: PS1, Line 219: *failed_instance_id = failed_instance_id_; > see header question -- this is why it seems we should require that the call Changed the semantic so that the caller must specify both or neither. PS1, Line 309: independently > independent of an instance Rewrote this comment. PS1, Line 310: incorporated > incorporated to where? and what is "here" referring to, given that you jus Rewrote this comment. PS1, Line 312: cumulative_status > that name confused me because it's doesn't seem like this is the cumulative Changed this to "overall_backend_status" and rewrote the comment. http://gerrit.cloudera.org:8080/#/c/7943/1/be/src/runtime/coordinator-backend-state.h File be/src/runtime/coordinator-backend-state.h: Line 95: /// (with not specific fragment responsible). > nit: s/not/no/? Done PS1, Line 100: has_failed_instance == false > what does that mean? do you mean '*has_failed_instance == false' or somethi Changed the semantics so that the caller must specify both or neither. Also updated the comment to make the semantics clear. http://gerrit.cloudera.org:8080/#/c/7943/1/be/src/runtime/coordinator.h File be/src/runtime/coordinator.h: PS1, Line 375: Status UpdateStatus(const Status& status, const TUniqueId& failed_fragment, : const std::string& instance_hostname, : bool has_failed_instance = true) WARN_UNUSED_RESULT; > If we keep this one method, let's be careful with our use of default parame Changed the ordering and made the arguments required. http://gerrit.cloudera.org:8080/#/c/7943/1/common/thrift/ImpalaInternalService.thrift File common/thrift/ImpalaInternalService.thrift: PS1, Line 628: This incorporates status from : // TFragmentInstanceExecStatus as well as errors that occur independently > given today's implementation it doesn't matter, but from a protocol standpo Updated the comment to be clear about the priorities. We don't currently report multiple errors, but I specified what I think is a reasonable semantic for that case. -- To view, visit http://gerrit.cloudera.org:8080/7943 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4cd98022f1d62a999c7c80ff5474fa8d069eb12c Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Joe McDonnell <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Philip Zeyliger <[email protected]> Gerrit-HasComments: Yes
