Marcel Kornacker has posted comments on this change. Change subject: IMPALA-2550: Switch to per-query exec rpc ......................................................................
Patch Set 11: (12 comments) http://gerrit.cloudera.org:8080/#/c/6535/10/be/src/runtime/coordinator.cc File be/src/runtime/coordinator.cc: PS10, Line 413: > In your current code, will the real status eventually be propagated as the clarified in query-state.h PS10, Line 424: > It gets displayed as soon as there is a client request state registered, I we might want to fix that. i'll remove that dcheck for now. http://gerrit.cloudera.org:8080/#/c/6535/11/be/src/runtime/coordinator.cc File be/src/runtime/coordinator.cc: PS11, Line 200: DCHECK(coord_instance_->WaitForPrepare().ok()); > DCHECK(coord_instance_->IsPrepared() && coord_instance_->GetPrepareStatus() Done http://gerrit.cloudera.org:8080/#/c/6535/11/be/src/runtime/query-exec-mgr.cc File be/src/runtime/query-exec-mgr.cc: PS11, Line 57: QueryState > QueryState reference Done PS11, Line 59: exec-fragment-instance > start-query-finstances Done http://gerrit.cloudera.org:8080/#/c/6535/10/be/src/runtime/query-state.cc File be/src/runtime/query-state.cc: PS10, Line 195: ImpalaBackendConnection coord(ExecEnv::GetInstance()->impalad_client_cache(), : query_ctx().coord_address, &coord_status); : if (!coord_status.ok()) { > Yes, flooding the log. i.e. the TODO would be better if it says what should the question is whether we want to cancel here if it's not the final report, ie, done==false. i'm happy to change it, it's not clear that one approach is better than the other. http://gerrit.cloudera.org:8080/#/c/6535/11/be/src/runtime/query-state.cc File be/src/runtime/query-state.cc: PS11, Line 211: if a single instance encountered an error, report an overall error status : // for the whole query to initiate cancellation at the coordinator > but why doesn't the coordinator initiate cancellation if any FIS reports er this status was actually unused. removed. PS11, Line 264: despire > despite Done http://gerrit.cloudera.org:8080/#/c/6535/10/be/src/runtime/query-state.h File be/src/runtime/query-state.h: PS10, Line 132: e(); > Agree we'll revisit this in a later change. Done http://gerrit.cloudera.org:8080/#/c/6535/11/be/src/runtime/query-state.h File be/src/runtime/query-state.h: PS11, Line 250: CancelInternal > that's no longer defined; delete. Done PS11, Line 260: cancel_on_error is true. > This is subtle as to why we have it so I think it would be helpful to expla Done http://gerrit.cloudera.org:8080/#/c/6535/11/be/src/runtime/test-env.cc File be/src/runtime/test-env.cc: Line 83: //exec_env_->disk_io_mgr_.reset(); > what's this about? Done -- To view, visit http://gerrit.cloudera.org:8080/6535 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I20769e420711737b6b385c744cef4851cee3facd Gerrit-PatchSet: 11 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Marcel Kornacker <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Henry Robinson <[email protected]> Gerrit-Reviewer: Marcel Kornacker <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
