Marcel Kornacker has posted comments on this change. Change subject: IMPALA-4014: Introduce query-wide execution state. ......................................................................
Patch Set 7: (15 comments) http://gerrit.cloudera.org:8080/#/c/4418/7/be/src/runtime/coordinator.cc File be/src/runtime/coordinator.cc: PS7, Line 938: ! > nit: missing space Done http://gerrit.cloudera.org:8080/#/c/4418/7/be/src/runtime/coordinator.h File be/src/runtime/coordinator.h: PS7, Line 507: fragment_instance_state > instance_state to match below and the definition. Done PS7, Line 511: fragment_instance_state > same Done http://gerrit.cloudera.org:8080/#/c/4418/7/be/src/runtime/fragment-instance-state.cc File be/src/runtime/fragment-instance-state.cc: Line 75: // including the final status when execution finishes. > I had deleted this comment because it wasn't accurate (and had added the ac rebase accident. removed. http://gerrit.cloudera.org:8080/#/c/4418/7/be/src/runtime/query-exec-mgr.cc File be/src/runtime/query-exec-mgr.cc: Line 115: // always decrement refcount > not sure what this comment is trying to tell me. would be more informative Done Line 148: qs = it->second; > it's probably worth commenting here explaining why we need to get a new qs Done http://gerrit.cloudera.org:8080/#/c/4418/7/be/src/runtime/query-exec-mgr.h File be/src/runtime/query-exec-mgr.h: Line 38: /// entry point for gaining refcounted access to a QueryState. > this doesn't really explain that it's also the thing that executes instance added comment. PS7, Line 47: decrement > this really happens after execution finishes, so would be good to reword. Done PS7, Line 53: CancelFInstance > what's that? (not a method of this class) Done http://gerrit.cloudera.org:8080/#/c/4418/7/be/src/runtime/query-state.h File be/src/runtime/query-state.h: PS7, Line 79: desc_tbl > do we plan to pull this out then? (maybe it's mentioned somewhere i haven't i expanded the comment. Line 83: bool released_resources() const { return released_resources_; } > who outside this class will care about this? besides, in order to call thes the idea with explicit resource release (as opposed to implicit, via some d'tor) is that we keep the control structures around (such as QueryState, FIS) but still get to release resources. in other words, we can release resources and then send the final exec report or do whatever else. the control structures will stay around until the last refcnt is gone. Line 93: /// Illegal to call any other function of this class after this call returns. > Isn't it more that a reference must be held when calling the other methods, see above. the second sentence is wrong. removed. http://gerrit.cloudera.org:8080/#/c/4418/7/be/src/runtime/runtime-state.cc File be/src/runtime/runtime-state.cc: Line 90: DCHECK(status.ok()) << status.GetDetail(); > not for this change but we should clean this up. actually, Init() always returns ok. i'm removing this dcheck. http://gerrit.cloudera.org:8080/#/c/4418/7/be/src/service/impala-internal-service.cc File be/src/service/impala-internal-service.cc: Line 68: fis->Cancel(); > is it intentional that we ignore the status returned by Cancel()? We used oversight. fixed. http://gerrit.cloudera.org:8080/#/c/4418/7/be/src/service/query-exec-state.h File be/src/service/query-exec-state.h: Line 55: /// TODO: Consider renaming to RequestExecState for consistency. > don't do it as part of this change, but this renaming is more important now i agree, it's getting confusing. -- To view, visit http://gerrit.cloudera.org:8080/4418 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I962ae6b7cb7dc0d07fbb8f70317aeb01d88d400b Gerrit-PatchSet: 7 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Marcel Kornacker <mar...@cloudera.com> Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com> Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com> Gerrit-Reviewer: Henry Robinson <he...@cloudera.com> Gerrit-Reviewer: Lars Volker <l...@cloudera.com> Gerrit-Reviewer: Marcel Kornacker <mar...@cloudera.com> Gerrit-Reviewer: Matthew Jacobs <m...@cloudera.com> Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com> Gerrit-HasComments: Yes
