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 <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Henry Robinson <[email protected]> Gerrit-Reviewer: Marcel Kornacker <[email protected]> Gerrit-HasComments: Yes
