Dan Hecht has posted comments on this change. Change subject: IMPALA-5892: Allow reporting status independent of fragment instance ......................................................................
Patch Set 1: (7 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 caller can only ask for the failed_instance_id if it also asks whether there is a failed instance. otherwise, this assignment is kinda meaningless. PS1, Line 309: independently independent of an instance (to clarify what it's independent of) PS1, Line 310: incorporated incorporated to where? and what is "here" referring to, given that you just said that the cumulative status includes fragment instance status. I had to read through the code to understand what the comment was saying, so I think the comment would be clearer if it said something like: So far 'status_' incorporates the fragment instances' status. Now incorporate the overall exec status into 'status_'. or something along those lines. PS1, Line 312: cumulative_status that name confused me because it's doesn't seem like this is the cumulative status, but instead status_ is the thing we are accumulating. I think in a comment later you refer to this as the "general status". That might be clearer to call this general_status. http://gerrit.cloudera.org:8080/#/c/7943/1/be/src/runtime/coordinator-backend-state.h File be/src/runtime/coordinator-backend-state.h: PS1, Line 100: has_failed_instance == false what does that mean? do you mean '*has_failed_instance == false' or something different? should we require that failed_instance_id != nullptr implies has_failed_instance != nullptr? 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; > Might be cleaner to add two functions UpdateFragmentStatus(status, failed_f If we keep this one method, let's be careful with our use of default parameter values. sometimes they are convenient but i think we should be careful to use them since they can often lead to errors due to forgetting to set the value. And in this case, it seems more intuitive to have has_failed_instance come before failed_fragement since the meaning of failed_fragment is dependent on it. And to be consistent in parameter order of GetStatus(), which is tightly related. 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 standpoint, if there are multiple instances with error status, which one is this set to? i.e. i think we should be very clear on our specification of our protocols. -- 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-HasComments: Yes
