Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/8758 )
Change subject: IMPALA-6190/6246: Add instances tab and event sequence ...................................................................... Patch Set 8: (6 comments) http://gerrit.cloudera.org:8080/#/c/8758/8/be/src/runtime/coordinator-backend-state.cc File be/src/runtime/coordinator-backend-state.cc: http://gerrit.cloudera.org:8080/#/c/8758/8/be/src/runtime/coordinator-backend-state.cc@626 PS8, Line 626: DCHECK_EQ(instance_stats.Size(), fragments_.size()); : value->AddMember("instance_stats", instance_stats, document->GetAllocator()); : : Value val(TNetworkAddressToString(impalad_address()).c_str(), document->GetAllocator()); : value->AddMember("host", val, document->GetAllocator() Same question. Can these lines be done outside of the lock ? http://gerrit.cloudera.org:8080/#/c/8758/4/be/src/runtime/coordinator.cc File be/src/runtime/coordinator.cc: http://gerrit.cloudera.org:8080/#/c/8758/4/be/src/runtime/coordinator.cc@1279 PS4, Line 1279: doc->AddMember("backend_instances", states, doc->GetAllocator()); Can this be done without holding the lock ? http://gerrit.cloudera.org:8080/#/c/8758/8/be/src/runtime/fragment-instance-state.h File be/src/runtime/fragment-instance-state.h: http://gerrit.cloudera.org:8080/#/c/8758/8/be/src/runtime/fragment-instance-state.h@172 PS8, Line 172: Emitted at the start of Prepare() after the : /// instance's event sequence has been set up. nit: just wondering if we can skip the "Emitted at ..." given it's quite obvious from the code. http://gerrit.cloudera.org:8080/#/c/8758/4/be/src/runtime/fragment-instance-state.cc File be/src/runtime/fragment-instance-state.cc: http://gerrit.cloudera.org:8080/#/c/8758/4/be/src/runtime/fragment-instance-state.cc@487 PS4, Line 487: > There are no updating threads, but there's still a race between updating it Can you please add a small comment to make the intention of AtomicEnum clear so readers will know that this function is not meant to be thread safe. http://gerrit.cloudera.org:8080/#/c/8758/8/be/src/runtime/fragment-instance-state.cc File be/src/runtime/fragment-instance-state.cc: http://gerrit.cloudera.org:8080/#/c/8758/8/be/src/runtime/fragment-instance-state.cc@248 PS8, Line 248: if (runtime_state_->ShouldCodegen()) { Should we start counting CODEGEN_START at this point instead after Prepare() has been called on the exec tree and the sink has been created ? Conceptually, it is a new phrase. In fact, I wonder if we should actually consider merging this with the similar if-stmt in Open(). http://gerrit.cloudera.org:8080/#/c/8758/8/be/src/util/runtime-profile-counters.h File be/src/util/runtime-profile-counters.h: http://gerrit.cloudera.org:8080/#/c/8758/8/be/src/util/runtime-profile-counters.h@338 PS8, Line 338: while (i < timestamps.size() && timestamps[i] <= last_timestamp) ++i; : for (; i < timestamps.size(); ++i) { Is this in the perf critical path ? Can we merge these two loops ? Seems easier to follow that way. -- To view, visit http://gerrit.cloudera.org:8080/8758 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I626456b6afa9101eeeeffd5cda10c4096d63d7f9 Gerrit-Change-Number: 8758 Gerrit-PatchSet: 8 Gerrit-Owner: Lars Volker <l...@cloudera.com> Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com> Gerrit-Reviewer: Joe McDonnell <joemcdonn...@cloudera.com> Gerrit-Reviewer: Lars Volker <l...@cloudera.com> Gerrit-Reviewer: Michael Ho <k...@cloudera.com> Gerrit-Comment-Date: Tue, 19 Dec 2017 22:42:12 +0000 Gerrit-HasComments: Yes
