Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11967 )
Change subject: IMPALA-1048: show sinks in exec summary ...................................................................... Patch Set 7: (7 comments) http://gerrit.cloudera.org:8080/#/c/11967/7/be/src/exec/plan-root-sink.cc File be/src/exec/plan-root-sink.cc: http://gerrit.cloudera.org:8080/#/c/11967/7/be/src/exec/plan-root-sink.cc@79 PS7, Line 79: while (results_ == nullptr && !state->is_cancelled()) sender_cv_.Wait(l); > Should we count the time spent here as inactive_time ? This seems to be the Yeah I like that idea. http://gerrit.cloudera.org:8080/#/c/11967/7/be/src/runtime/coordinator.cc File be/src/runtime/coordinator.cc: http://gerrit.cloudera.org:8080/#/c/11967/7/be/src/runtime/coordinator.cc@234 PS7, Line 234: optional in the thrift > Should they become required then ? I'm not sure about the schema evolution rules - I think it's probably ok to switch an optional field to required if all of the producers were already setting it. I actually kind of think that optional is the right thing and it's a bug/oversight in impala-shell that it doesn't handle this properly. Maybe the ideal approach would be to fix impala-shell and at some point later make the corresponding change here, once we think it's unlikely that older versions of impala-shell will be used against the Impala version. I can file a JIRA and add a comment here if you agree. (I don't think we can be confident that older versions of impala-shell won't be used against a newer cluster, although I don't think we've ever had a clear compatibility policy stating that this should work) http://gerrit.cloudera.org:8080/#/c/11967/7/be/src/util/runtime-profile.h File be/src/util/runtime-profile.h: http://gerrit.cloudera.org:8080/#/c/11967/7/be/src/util/runtime-profile.h@307 PS7, Line 307: plan node > data sink Done http://gerrit.cloudera.org:8080/#/c/11967/7/be/src/util/runtime-profile.cc File be/src/util/runtime-profile.cc: http://gerrit.cloudera.org:8080/#/c/11967/7/be/src/util/runtime-profile.cc@962 PS7, Line 962: void RuntimeProfile::SetPlanNodeId(int node_id) { > Should we DCHECK that "data_sink_id" is not set here ? Done http://gerrit.cloudera.org:8080/#/c/11967/7/be/src/util/runtime-profile.cc@966 PS7, Line 966: void RuntimeProfile::SetDataSinkId(int sink_id) { > Should we DCHECK that "plan_node_id" is not set here ? Done http://gerrit.cloudera.org:8080/#/c/11967/7/be/src/util/summary-util.cc File be/src/util/summary-util.cc: http://gerrit.cloudera.org:8080/#/c/11967/7/be/src/util/summary-util.cc@79 PS7, Line 79: LOG(INFO) << label_ss.str() << " " << node.node_id; > Is this for debugging only ? Done http://gerrit.cloudera.org:8080/#/c/11967/7/common/thrift/RuntimeProfile.thrift File common/thrift/RuntimeProfile.thrift: http://gerrit.cloudera.org:8080/#/c/11967/7/common/thrift/RuntimeProfile.thrift@67 PS7, Line 67: // Set if this node corresponds to a plan node. : 1: optional i32 plan_node_id : : // Set if this node corresponds to a data sink. : 2: optional i32 data_sink_id > Are these two not supposed to be set at the same time ? if so, can you plea Done -- To view, visit http://gerrit.cloudera.org:8080/11967 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3fdf7bacae8ff597b255da65af453e174ba53544 Gerrit-Change-Number: 11967 Gerrit-PatchSet: 7 Gerrit-Owner: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Lars Volker <l...@cloudera.com> Gerrit-Reviewer: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: Philip Zeyliger <phi...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Thu, 29 Nov 2018 02:26:54 +0000 Gerrit-HasComments: Yes
