Michael Ho 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 convention for other plan nodes and DataStreamSender. Didn't dive into other hdfs table writer sinks to see if inactive time also applies there. 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 ? 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 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 ? 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 ? 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 ? 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 please add a comment ? -- 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 <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Philip Zeyliger <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Wed, 28 Nov 2018 23:53:32 +0000 Gerrit-HasComments: Yes
