impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Marcel Kornacker (Code Review)" <ger...@cloudera.org>
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:

(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 <marcel@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Henry Robinson <henry@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <marcel@cloudera.com>
Gerrit-Reviewer: Michael Ho <kwho@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message