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: (2 comments) http://gerrit.cloudera.org:8080/#/c/10813/2/be/src/runtime/query-state.cc File be/src/runtime/query-state.cc: http://gerrit.cloudera.org:8080/#/c/10813/2/be/src/runtime/query-state.cc@437 PS2, Line 437: : } : VLOG_QUERY << "descriptor table for query=" << PrintId(query_id()) : << "\n" << desc_tbl_->DebugString(); : : Status thread_create_status; : DCHECK_GT(rpc_params_.fragment_ctxs.size(), 0); : TPlanFragmentCtx* fragment_ctx = &rpc_params_.fragment_ctxs[0]; : int fragment_ctx_idx = 0; : fragment_events_start_time_ = MonotonicStopWatch::Now(); : for (const TPlanFragmentInstanceCtx& instance_ctx: rpc_params_.fragment_instance_ctxs) { : // determine corresponding TPlanFragmentCtx : if (fragment_ctx->fragment.idx != instance_ctx.fragment_idx) { : ++fragment_ctx_idx; : > My point was shouldn't the counting barrier take care of that for us? I.e. CountingBarrier::Notify() does not set the promise unless it's the one to reduce the count to 0 (or in other words only the last fragment instance to complete preparing would get to set the promise). So if a first fragment instance hits an error in Prepare() while the others are still running Prepare(), then it won't be able to set the error unless it has some way of communicating its error to the other fragment instances, or it waits until the rest of them are done and then sets the error. So, we'd need some other mechanism of allowing us to both: 1) Save the first error, and 2) Wait for all the FInstances to complete Prepare() Unless I'm missing something? http://gerrit.cloudera.org:8080/#/c/10813/2/be/src/runtime/query-state.cc@586 PS2, Line 586: : : : : : > Right, the references are held by FIS for resources shared across the query As we spoke in person, I will roll this part out as a separate patch to make reviewing this a bit easier. -- 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 17:00:35 +0000 Gerrit-HasComments: Yes
