impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dan Hecht (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-5892: Allow reporting status independent of fragment instance
Date Fri, 01 Sep 2017 23:20:53 GMT
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 <joemcdonnell@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <joemcdonnell@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message