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 Thu, 13 Apr 2017 23:22:02 GMT
Marcel Kornacker has posted comments on this change.

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


Patch Set 4:

(19 comments)

http://gerrit.cloudera.org:8080/#/c/6535/4/be/src/runtime/coordinator-backend-state.cc
File be/src/runtime/coordinator-backend-state.cc:

PS4, Line 208: #if 0
> huh ?
removed


http://gerrit.cloudera.org:8080/#/c/6535/4/be/src/runtime/coordinator.cc
File be/src/runtime/coordinator.cc:

PS4, Line 424: VLOG_QUERY << "set new status";
> Debugging output ?
yes, i'll clean all that up before it goes in.


PS4, Line 632:  //
> Did you mean to comment this line out ?
Done


Line 714:     if (!partition_create_ops.Execute(ExecEnv::GetInstance()->hdfs_op_thread_pool(),
false)) {
> long line
Done


Line 976: #if 0
> huh ?
i think henry put there, i'd like to hear from someone whether we need to keep it.


http://gerrit.cloudera.org:8080/#/c/6535/4/be/src/runtime/coordinator.h
File be/src/runtime/coordinator.h:

PS4, Line 216: //ExecSummary* exec_summary() { return &exec_summary_; }
> Is this not needed ?
Done


PS4, Line 304: lock_
> The lock acquisition order is still unclear wrt other locks in this class (
agreed, this needs to be further simplified and clarified, but it's out of scope for this
already very large change.

tests: existing stress


PS4, Line 404: before
> Mind explaining why it must be called before InitBackendState()  in the com
Done


PS4, Line 444:  
> blank space
Done


http://gerrit.cloudera.org:8080/#/c/6535/4/be/src/runtime/descriptors.cc
File be/src/runtime/descriptors.cc:

PS4, Line 500: partition_key_value_ctxs
> Isn't the partition expr Literal though ? Does it really need to allocate m
it doesn't, but i didn't want to mess with exprctx::prepare too much, it uses tracker to create
a new pool.

i'll leave a todo to clean this up.


http://gerrit.cloudera.org:8080/#/c/6535/4/be/src/runtime/fragment-instance-state.h
File be/src/runtime/fragment-instance-state.h:

PS4, Line 91:  
> blank space
Done


http://gerrit.cloudera.org:8080/#/c/6535/4/be/src/runtime/query-state.cc
File be/src/runtime/query-state.cc:

PS4, Line 254: 100
> magic constant. Could this be better to be #define or constexpr value ?
Done


http://gerrit.cloudera.org:8080/#/c/6535/4/be/src/runtime/runtime-state.cc
File be/src/runtime/runtime-state.cc:

PS4, Line 105: test-pool
> What does "test-pool" mean ?
nothing specific. that's the name we've been using for tests.


http://gerrit.cloudera.org:8080/#/c/6535/4/be/src/runtime/runtime-state.h
File be/src/runtime/runtime-state.h:

PS4, Line 99: ObjectPoool
> typo.
Done


http://gerrit.cloudera.org:8080/#/c/6535/4/be/src/service/impala-hs2-server.cc
File be/src/service/impala-hs2-server.cc:

PS4, Line 194:   // Make sure ClientRequestState::Wait() has completed before fetching rows.
Wait() ensures
             :   // that rows are ready to be fetched (e.g., Wait() opens ClientRequestState::output_exprs_,
> long lines
Done


http://gerrit.cloudera.org:8080/#/c/6535/4/be/src/util/uid-util.h
File be/src/util/uid-util.h:

PS4, Line 81: IsValidId
> IsValidFid ? It's not entirely clear from the function name what Id is.
Done


http://gerrit.cloudera.org:8080/#/c/6535/4/common/thrift/ImpalaInternalService.thrift
File common/thrift/ImpalaInternalService.thrift:

PS4, Line 427: TExecQueryFInstancesParams 
> Can you please add some brief comments on the overview of how these structu
Done


PS4, Line 545: V1
> What does V1 stand for ? Version 1 ? of what ?
ImpalaInternalServiceVersion


PS4, Line 698: required
> Is it optional or required ?
in rpc structs, all parameters other than the protocol version need to be optional, otherwise
you can't change them later. but for a specific version, a field can still be required.


-- 
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: 4
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-HasComments: Yes

Mime
View raw message