impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dan Hecht (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-2550: Switch to per-query exec rpc
Date Tue, 04 Apr 2017 23:58:34 GMT
Dan Hecht has posted comments on this change.

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


Patch Set 3:

(14 comments)

Just looked at the thrift so far.

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

PS3, Line 381: the total number
             : // of fragment instances
what's that?


PS3, Line 386: i32
Types.TFragmentIdx ?


PS3, Line 412: 6: list<TPlanFragmentDestination> destinations
isn't that common across all instances?


PS3, Line 414: fragment
instance?


PS3, Line 435: coord_state_idx
not clear why the backend will care what index it was represented by in a coordinator's datastructure.
why is that?
oh, reading on I see this is really just used as to identify the backend when calling back
via ReportExecStatus. it could use a little explanation.


Line 441:   4: list<TPlanFragmentCtx> fragment_ctxs
are these in any particular order?


Line 453: // ReportExecStatus
other RPC names have a blank line after them, which makes them easier to find.


PS3, Line 552: overall
what does "overall" mean here?  this makes it seem like it's the merged status of all the
instances, but from reading the code it's not.


PS3, Line 560: insert_exec_status
I guess this will hold the insert status for all instances, right?


Line 563:   7: optional map<ErrorCodes.TErrorCode, TErrorLogEntry> error_log;
and same here. this will be the aggregated error log across all instances?


PS3, Line 572: CancelPlanFragment
CancelQueryFInstances


Line 669: 
It would be helpful to add the RPC comment to show which RPC the following structures below
to (like we have above):

// UpdateFilter


Line 699: 
// PublishFilter


PS3, Line 734: ReportExecStatus
Eventually this will report status for all the instances (at least in the periodic case),
right? So it might be clearer to name it similar to the others: ReportFInstancesStatus() or
similar.


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

Mime
View raw message