impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Marcel Kornacker (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-2550: Switch to per-query exec rpc
Date Mon, 08 May 2017 01:55:11 GMT
Marcel Kornacker has posted comments on this change.

Change subject: IMPALA-2550: Switch to per-query exec rpc

Patch Set 11:

File be/src/runtime/

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.
File be/src/runtime/

PS11, Line 200: DCHECK(coord_instance_->WaitForPrepare().ok());
> DCHECK(coord_instance_->IsPrepared() && coord_instance_->GetPrepareStatus()
File be/src/runtime/

PS11, Line 57: QueryState
> QueryState reference

PS11, Line 59: exec-fragment-instance
> start-query-finstances
File be/src/runtime/

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.
File be/src/runtime/

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
File be/src/runtime/query-state.h:

PS10, Line 132: e();
> Agree we'll revisit this in a later change.
File be/src/runtime/query-state.h:

PS11, Line 250: CancelInternal
> that's no longer defined; delete.

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
File be/src/runtime/

Line 83:   //exec_env_->disk_io_mgr_.reset();
> what's this about?

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I20769e420711737b6b385c744cef4851cee3facd
Gerrit-PatchSet: 11
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Marcel Kornacker <>
Gerrit-Reviewer: Dan Hecht <>
Gerrit-Reviewer: Henry Robinson <>
Gerrit-Reviewer: Marcel Kornacker <>
Gerrit-Reviewer: Michael Ho <>
Gerrit-Reviewer: Tim Armstrong <>
Gerrit-HasComments: Yes

View raw message