impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Michael Ho (Code Review)" <ger...@cloudera.org>
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. ( 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 <lv@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <joemcdonnell@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv@cloudera.com>
Gerrit-Reviewer: Michael Ho <kwho@cloudera.com>
Gerrit-Comment-Date: Tue, 19 Dec 2017 22:42:12 +0000
Gerrit-HasComments: Yes

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