Michael Ho has posted comments on this change. Change subject: IMPALA-2550: Switch to per-query exec rpc ......................................................................
Patch Set 6: (7 comments) http://gerrit.cloudera.org:8080/#/c/6535/4/be/src/exec/data-sink.cc File be/src/exec/data-sink.cc: PS4, Line 66: > how so? We use indentation 4 for line continuation. See line 119 below. 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 184: : if (!rpc_status.ok()) { > out of scope for this patch. how is this out of scope ? Line 365: TPublishFilterResult res; > i left that in there as a reminder to myself to think this through again. At which version of this patch will this be thought through ? PS4, Line 391: > IsDone() simply returns a bool, not sure what you mean. *done = IsDone(); PS4, Line 434: > it wasn't there before, and it has the potential to spam the log. I don't see how this can spam the log if it's not expected to occur frequently. If it happens very frequently, then there is something wrong. http://gerrit.cloudera.org:8080/#/c/6535/4/common/thrift/ImpalaInternalService.thrift File common/thrift/ImpalaInternalService.thrift: PS4, Line 399: across > there are two indices, which come in handy in different situation: So, is this a per fragment index different from the globally unique instance index ? Can you provide more clarification on how each of them is used and add a remark they are really different. It's important to be as clear as possible to avoid confusion. PS4, Line 400: num_fragment_instances I don't see a field called num_fragment_instances in TPlanFragmentCtx defined above. -- 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: 6 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-Reviewer: Michael Ho <[email protected]> Gerrit-HasComments: Yes
