impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Michael Ho (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-6190/6246: Add instances tab and event sequence
Date Tue, 19 Dec 2017 22:42:12 GMT
Michael Ho has posted comments on this change. ( )

Change subject: IMPALA-6190/6246: Add instances tab and event sequence

Patch Set 8:

File be/src/runtime/
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 ?
File be/src/runtime/
PS4, Line 1279: doc->AddMember("backend_instances", states, doc->GetAllocator());
Can this be done without holding the lock ?
File be/src/runtime/fragment-instance-state.h:
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
File be/src/runtime/
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.
File be/src/runtime/
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
File be/src/util/runtime-profile-counters.h:
PS8, Line 338:     while (i < timestamps.size() && timestamps[i] <= last_timestamp)
             :     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
To unsubscribe, visit

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 <>
Gerrit-Reviewer: Dan Hecht <>
Gerrit-Reviewer: Joe McDonnell <>
Gerrit-Reviewer: Lars Volker <>
Gerrit-Reviewer: Michael Ho <>
Gerrit-Comment-Date: Tue, 19 Dec 2017 22:42:12 +0000
Gerrit-HasComments: Yes

  • Unnamed multipart/alternative (inline, 8-Bit, 0 bytes)
View raw message