Lars Volker has posted comments on this change. Change subject: IMPALA-4014: Introduce query-wide execution state. ......................................................................
Patch Set 11: (20 comments) http://gerrit.cloudera.org:8080/#/c/3817/11/be/src/runtime/fragment-instance-exec-state.h File be/src/runtime/fragment-instance-exec-state.h: Line 54: Status Prepare(); > yet more indirection. Could we just make them one class instead? http://gerrit.cloudera.org:8080/#/c/3817/11/be/src/runtime/fragment-instance-executor.h File be/src/runtime/fragment-instance-executor.h: PS11, Line 49: fragment fragment instance? PS11, Line 50: fragment instance? PS11, Line 53: fragment instance? PS11, Line 85: fragment instance? PS11, Line 88: fragment instance? Line 145: QueryState* query_exec_state() { return query_exec_state_; } > now that we're doing a global rename, let's drop 'state' uniformly drop _exec_? http://gerrit.cloudera.org:8080/#/c/3817/11/be/src/runtime/query-exec-state.h File be/src/runtime/query-exec-state.h: PS11, Line 46: the the double word Line 115: int32_t num_current_references_; > not atomic? (or does it even make a difference?) Maybe also comment that this is changed by the QueryExecMgr directly (or add inc() and dec() method), so it doesn't look like an unused member. http://gerrit.cloudera.org:8080/#/c/3817/11/be/src/runtime/runtime-state.h File be/src/runtime/runtime-state.h: PS11, Line 65: /// A collection of items that are part of the global state of a : /// query and shared across all execution nodes of that query. How is this different from QueryState now? Is there a way to merge them? If not, can we elaborate in the comment? http://gerrit.cloudera.org:8080/#/c/3817/11/be/src/service/query-exec-mgr.cc File be/src/service/query-exec-mgr.cc: Line 42: Status QueryExecMgr::RegisterFragmentInstanceWithQuery( > this does more than just registration (it starts a thread that executes the In case this discussion hasn't happened, could you find a time where I can participate? Thanks. PS11, Line 133: query_state_map_ DCHECK that it's in the map? http://gerrit.cloudera.org:8080/#/c/3817/11/be/src/service/query-exec-mgr.h File be/src/service/query-exec-mgr.h: PS11, Line 52: executed Will it also execute it? PS11, Line 59: After this call returns, it is legal Are there cases where it is illegal to call CancelPlanFragment() on the fragment instance? PS11, Line 66: The caller Maybe say "Every caller" so it is more clear that each call to GQS() has to be matched by a call to RQS()? PS11, Line 91: should always check maybe add "check before the first usage", for subsequent calls guarantee 1. sounded like it will always be != nullptr. PS11, Line 96: query_exec_mgr_ This member doesn't exists. But DCHECK that ExecEnv::GetQueryExecMgr() exists sounds like a good idea (also in the d'tor). PS11, Line 97: ExecEnv Missing call to GetInstance(), also in d'tor. PS11, Line 123: ThreadPool<QueryState*> destroy_thread_; The difference between the type and variable name makes this harder to understand for me. PS11, Line 137: Calls ReleaseQueryState() Where does the corresponding increase happen? -- To view, visit http://gerrit.cloudera.org:8080/3817 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I892091d6401acb2ea91ccb1623af54c6f9635e6c Gerrit-PatchSet: 11 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Sailesh Mukil <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: David Knupp <[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
