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 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:

File be/src/runtime/

PS4, Line 208: #if 0
> huh ?
File be/src/runtime/

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 ?

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

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

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

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

PS4, Line 444:  
> blank space
File be/src/runtime/

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

PS4, Line 91:  
> blank space
File be/src/runtime/

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

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

PS4, Line 99: ObjectPoool
> typo.
File be/src/service/

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
File be/src/util/uid-util.h:

PS4, Line 81: IsValidId
> IsValidFid ? It's not entirely clear from the function name what Id is.
File common/thrift/ImpalaInternalService.thrift:

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

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

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
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I20769e420711737b6b385c744cef4851cee3facd
Gerrit-PatchSet: 4
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-HasComments: Yes

View raw message