Dan Hecht has posted comments on this change. Change subject: IMPALA-4014: Introduce query-wide execution state. ......................................................................
Patch Set 7: (18 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 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. PS7, Line 511: fragment_instance_state same 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 accurate info the the header comment). Let's not add it back. http://gerrit.cloudera.org:8080/#/c/4418/7/be/src/runtime/fragment-instance-state.h File be/src/runtime/fragment-instance-state.h: Line 53: /// - move ReportStatusCb() logic into PFE::SendReport() and get rid of the callback thanks, these four are going to make things a lot better. 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 as: decrement the refcount taken in StartFInstance() Line 148: qs = it->second; it's probably worth commenting here explaining why we need to get a new qs pointer, i.e. someone else may have gc'd the old one and then someone else created a new one. it's as subtle as the other cases that are commented. 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 instances. PS7, Line 47: decrement this really happens after execution finishes, so would be good to reword. PS7, Line 53: CancelFInstance what's that? (not a method of this class) 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 read yet). Line 83: bool released_resources() const { return released_resources_; } who outside this class will care about this? besides, in order to call these methods, you have to have a refcnt, which means the resources couldn't have been released, right? 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, and that this method can't be called until all references are returned? why not specify that? 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. Line 345: } are any of these on critical paths (i.e. does the call instruction you're adding matter)? http://gerrit.cloudera.org:8080/#/c/4418/7/be/src/runtime/runtime-state.h File be/src/runtime/runtime-state.h: Line 354: /// TODO: get rid of this and use ExecEnv::GetInstance() instead thanks 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 to propagate back as the return_val status (inside CancelPlanFragment). 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 given the other similarly named class. -- 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 <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Henry Robinson <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Reviewer: Marcel Kornacker <[email protected]> Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-Reviewer: Sailesh Mukil <[email protected]> Gerrit-HasComments: Yes
