Lars Volker 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 9: (6 comments) Thanks for the comments, please see PS9. 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()); : : // impalad_address is not protected by lock_. The life > Same question. Can these lines be done outside of the lock ? Done, though the pattern of this file seems to be "hold the lock for the duration of the ToJson() method" (see above). I convinced myself that it is safe to release the lock early and added a comment. 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: state->InstanceStatsToJson(&val, doc); > Can this be done without holding the lock ? Done 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: : PREPARE_START, > nit: just wondering if we can skip the "Emitted at ..." given it's quite ob Done 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: > Can you please add a small comment to make the intention of AtomicEnum clea Done 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( Done, I moved all of the codegen to here. Let me know if you prefer to move it to 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: if (timestamps[i] <= last_timestamp) continue; : events_.push_back(make_pair(labels > Is this in the perf critical path ? Can we merge these two loops ? Seems ea Done. We could also do if (timestamps[i] > last_timestamp) { events_.push_back(...); } Let me know which one you prefer. -- 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: 9 Gerrit-Owner: Lars Volker <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Comment-Date: Wed, 20 Dec 2017 13:19:58 +0000 Gerrit-HasComments: Yes
