Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/10813 )
Change subject: IMPALA-7163: Implement a state machine for the QueryState class ...................................................................... Patch Set 4: (19 comments) http://gerrit.cloudera.org:8080/#/c/10813/3/be/src/runtime/query-state.h File be/src/runtime/query-state.h: http://gerrit.cloudera.org:8080/#/c/10813/3/be/src/runtime/query-state.h@65 PS3, Line 65: state denot > I think we should avoid using the term "query state" given that this class Done http://gerrit.cloudera.org:8080/#/c/10813/3/be/src/runtime/query-state.h@65 PS3, Line 65: dExecState. We > Backend Done http://gerrit.cloudera.org:8080/#/c/10813/3/be/src/runtime/query-state.h@68 PS3, Line 68: to the ERROR or CANCELLED > are there multiple error states? corresponding implies that. Or are you inc I reworded it to be clearer now. It can only either be ERROR or CANCELLED. http://gerrit.cloudera.org:8080/#/c/10813/3/be/src/runtime/query-state.h@72 PS3, Line 72: This is to simplify the : /// query lifecycle so that Prepare() is always completed befor > Let's not state in terms of past code. How about: Yes, technically Cancel() or PublishFilter(). So I've re-worded it to that. http://gerrit.cloudera.org:8080/#/c/10813/3/be/src/runtime/query-state.h@149 PS3, Line 149: s belonging to this query. Each instance receives its own execution > put another way, I guess this blocks until threads have exited INITIALIZED? Yes, I've added that wording to make it clearer. The reason I kept MonitorFInstances() separately is because the status reporting will be driven from there after IMPALA-2990, making it more readable than keeping it in StartFInstances(). http://gerrit.cloudera.org:8080/#/c/10813/3/be/src/runtime/query-state.h@155 PS3, Line 155: /// Monitors all the fragment instances and updates the BackendExecState according > it it required that this is called after StartFInstances()? Yes, I've added that to the comments now. http://gerrit.cloudera.org:8080/#/c/10813/3/be/src/runtime/query-state.h@210 PS3, Line 210: return strings::Substitute("Status: $0 (fra > Is that needed in C++? You'd do it for C but I think C++ effectively inclu You're right. Removed it. http://gerrit.cloudera.org:8080/#/c/10813/3/be/src/runtime/query-state.h@213 PS3, Line 213: > nit: here and below you could delete the word "Function" since that's obvio Done http://gerrit.cloudera.org:8080/#/c/10813/3/be/src/runtime/query-state.h@266 PS3, Line 266: dExecS > Isn't Cancel() called in case of error as well? If so, does this mean we go Yes, that's true. I've reworded it to specify only CancelQueryFInstances() RPC and a coordinator directed CANCEL from a response to ReportExecStatus() RPC. http://gerrit.cloudera.org:8080/#/c/10813/3/be/src/runtime/query-state.h@271 PS3, Line 271: repare(). Implies > how about backend_exec_state_ for consistency? Done http://gerrit.cloudera.org:8080/#/c/10813/3/be/src/runtime/query-state.h@284 PS3, Line 284: u > here and elsewhere: given BackendExecState is a scalar (enum) value, should Done http://gerrit.cloudera.org:8080/#/c/10813/3/be/src/runtime/query-state.h@286 PS3, Line 286: or E > from Done http://gerrit.cloudera.org:8080/#/c/10813/3/be/src/runtime/query-state.h@305 PS3, Line 305: ar* Ba > overall status Done http://gerrit.cloudera.org:8080/#/c/10813/3/be/src/runtime/query-state.h@306 PS3, Line 306: > otherwise it's the status of the first non-OK FIS? Yes, added that to the comment. Also, I changed this to a FInstanceStatus. I wanted to do that initially, but I forgot to. http://gerrit.cloudera.org:8080/#/c/10813/3/be/src/runtime/query-state.cc File be/src/runtime/query-state.cc: http://gerrit.cloudera.org:8080/#/c/10813/3/be/src/runtime/query-state.cc@201 PS3, Line 201: const char* QueryState::BackendExecStateToString(const BackendExecState& state) { : static const unordered_map<BackendExecState, const char*> exec_state_to_str{ : {BackendExecState::INITIALIZED, "INITIALIZED"}, : {BackendExecState::PREPARED, "PREPARED"}, {BackendExecState::FINISHED, "FINISHED"}, : {BackendExecState::CANCELLED, "CANCELLED"}, {BackendExecState::ERROR, "ERROR"}}; : retu > given that the function is inlined this is probably unnecessary. dead code Good point. Done. http://gerrit.cloudera.org:8080/#/c/10813/3/be/src/runtime/query-state.cc@226 PS3, Line 226: << BackendExecStateToString(old_state) << ")"; > why doesn't that include FINISHED? This check doesn't seem very intuitive t After removing the 2 states I had before, this seems pretty unnecessary now. I removed it. http://gerrit.cloudera.org:8080/#/c/10813/3/be/src/runtime/query-state.cc@266 PS3, Line 266: ested cancellati > how about UpdateBackendExecState (since QueryState is the name of the class Done http://gerrit.cloudera.org:8080/#/c/10813/3/be/src/runtime/query-state.cc@290 PS3, Line 290: > how do we get into the cancelled state? Today, we don't enter into a CANCELLD state, but we will after IMPALA-2990. This is because the status reporting is per fragment instance now, and if the coordinator asks a fragment instance to cancel itself, it will call Cancel() as part of ReportExecStatus() in L412 (PS 3), which will set the cancelled flag in every fragment instance. And one of these fragment instances will be the first to call ErrorDuringExecute() which will set the 'query_status_' to CANCELLED but the 'backend_exec_state_' to ERROR (L277 - L 280). We can have special logic to handle this, but it will be pretty unnecessary after IMPALA-2990. After IMPALA-2990, the QueryState thread will be the first one to realize cancellation always and set the 'backend_exec_state_' to CANCELLED. http://gerrit.cloudera.org:8080/#/c/10813/3/be/src/runtime/query-state.cc@457 PS3, Line 457: ne > You shouldn't call stuff with side effects inside DCHECK. In release builds Done -- To view, visit http://gerrit.cloudera.org:8080/10813 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iec5670a7db83ecae4656d7bb2ea372d3767ba7fe Gerrit-Change-Number: 10813 Gerrit-PatchSet: 4 Gerrit-Owner: Sailesh Mukil <sail...@cloudera.com> Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com> Gerrit-Reviewer: Joe McDonnell <joemcdonn...@cloudera.com> Gerrit-Reviewer: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com> Gerrit-Comment-Date: Tue, 03 Jul 2018 16:49:31 +0000 Gerrit-HasComments: Yes
