You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "anujphadke (Code Review)" <ge...@cloudera.org> on 2016/10/05 18:40:56 UTC

[Impala-ASF-CR] IMPALA-3342: Adding new timer to accurately measure the TotalCpuTime 

anujphadke has uploaded a new change for review.

  http://gerrit.cloudera.org:8080/4633

Change subject: IMPALA-3342: Adding new timer to accurately measure the TotalCpuTime
......................................................................

IMPALA-3342: Adding new timer to accurately measure the TotalCpuTime

The plan fragment execution CPU timer is calculated
correctly. It subtracts the disk IO and network transmit time from the
total wall clock time which might not be very accurate. Adding a new
getrusage based timer for calcuating the CPU time.

Change-Id: Ifa88aa6f3371fa42d11ecc122f43c7d83623c300
---
M be/src/runtime/plan-fragment-executor.cc
M be/src/runtime/plan-fragment-executor.h
M be/src/util/stopwatch.h
3 files changed, 56 insertions(+), 7 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/33/4633/1
-- 
To view, visit http://gerrit.cloudera.org:8080/4633
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: Ifa88aa6f3371fa42d11ecc122f43c7d83623c300
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: anujphadke <ap...@cloudera.com>

[Impala-ASF-CR] Impala-3342 Adding thread counters to measure time spent during plan fragment execution

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.

Change subject: Impala-3342 Adding thread counters to measure time spent during plan fragment execution
......................................................................


Patch Set 2:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/4633/2//COMMIT_MSG
Commit Message:

Line 14: 
What does the profile look like? Would be helpful to see what the plan fragment profile looks like.


http://gerrit.cloudera.org:8080/#/c/4633/2/be/src/exec/hdfs-scan-node.cc
File be/src/exec/hdfs-scan-node.cc:

PS2, Line 360: SCOPED_THREAD_COUNTER_MEASUREMENT(runtime_state_->plan_fragment_counters());
> Is this thread safe?
Yep, scanner_thread_counters() is also shared between scanner threads.


http://gerrit.cloudera.org:8080/#/c/4633/2/be/src/runtime/plan-fragment-executor.cc
File be/src/runtime/plan-fragment-executor.cc:

Line 213:       ADD_COUNTER(profile(), PER_HOST_PEAK_MEM_COUNTER, TUnit::BYTES);
Let's create the new counter here along with the other ones so that the setup happens in the same place.


http://gerrit.cloudera.org:8080/#/c/4633/2/be/src/runtime/runtime-state.h
File be/src/runtime/runtime-state.h:

Line 241:   ///Fragment thread counters
The comment isn't too helpful. Maybe:

  /// Total CPU utilization for all threads in this plan fragment.


-- 
To view, visit http://gerrit.cloudera.org:8080/4633
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifa88aa6f3371fa42d11ecc122f43c7d83623c300
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: anujphadke <ap...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Yonghyun Hwang
Gerrit-HasComments: Yes

[Impala-ASF-CR] Impala-3342 Adding thread counters to measure time spent during plan fragment execution

Posted by "anujphadke (Code Review)" <ge...@cloudera.org>.
anujphadke has uploaded a new patch set (#2).

Change subject: Impala-3342 Adding thread counters to measure time spent during plan fragment execution
......................................................................

Impala-3342 Adding thread counters to measure time spent during plan
fragment execution

This change removes the use of total_cpu_timer which incorrectly
monitors the CPU time. Adding THREAD_COUNTERS to meausure the user
and sys time in plan fragment execution. This also accounts for the
time spent in the hdfs/kudu scanner and in a blocking join.

Change-Id: Ifa88aa6f3371fa42d11ecc122f43c7d83623c300
---
M be/src/exec/blocking-join-node.cc
M be/src/exec/hdfs-scan-node.cc
M be/src/exec/kudu-scan-node.cc
M be/src/runtime/plan-fragment-executor.cc
M be/src/runtime/plan-fragment-executor.h
M be/src/runtime/runtime-state.cc
M be/src/runtime/runtime-state.h
7 files changed, 16 insertions(+), 26 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/33/4633/2
-- 
To view, visit http://gerrit.cloudera.org:8080/4633
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ifa88aa6f3371fa42d11ecc122f43c7d83623c300
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: anujphadke <ap...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Yonghyun Hwang

[Impala-ASF-CR] IMPALA-3342: Add thread counters to monitor plan fragment execution

Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has posted comments on this change.

Change subject: IMPALA-3342: Add thread counters to monitor plan fragment execution
......................................................................


Patch Set 9: Code-Review+2

Looks good to me.

-- 
To view, visit http://gerrit.cloudera.org:8080/4633
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifa88aa6f3371fa42d11ecc122f43c7d83623c300
Gerrit-PatchSet: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: anujphadke <ap...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Yonghyun Hwang
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] Impala-3342: Adding thread counters to measure time spent during plan fragment execution

Posted by "anujphadke (Code Review)" <ge...@cloudera.org>.
anujphadke has uploaded a new patch set (#5).

Change subject: Impala-3342: Adding thread counters to measure time spent during plan fragment execution
......................................................................

Impala-3342: Adding thread counters to measure time spent during plan
fragment execution

This change removes the use of total_cpu_timer which incorrectly
monitors the CPU time. Adding THREAD_COUNTERS to measure the user
and sys time in plan fragment execution. This also accounts for the
time spent in the hdfs/kudu scanner and in a blocking join.

Change-Id: Ifa88aa6f3371fa42d11ecc122f43c7d83623c300
---
M be/src/exec/blocking-join-node.cc
M be/src/exec/hdfs-scan-node.cc
M be/src/exec/kudu-scan-node.cc
M be/src/runtime/plan-fragment-executor.cc
M be/src/runtime/plan-fragment-executor.h
M be/src/runtime/runtime-state.cc
M be/src/runtime/runtime-state.h
7 files changed, 34 insertions(+), 25 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/33/4633/5
-- 
To view, visit http://gerrit.cloudera.org:8080/4633
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ifa88aa6f3371fa42d11ecc122f43c7d83623c300
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: anujphadke <ap...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Yonghyun Hwang

[Impala-ASF-CR] IMPALA-3342: Add thread counters to monitor plan fragment execution

Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change.

Change subject: IMPALA-3342: Add thread counters to monitor plan fragment execution
......................................................................


Patch Set 9: Code-Review+1

Looks okay to me once Henry signs off.

-- 
To view, visit http://gerrit.cloudera.org:8080/4633
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifa88aa6f3371fa42d11ecc122f43c7d83623c300
Gerrit-PatchSet: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: anujphadke <ap...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Yonghyun Hwang
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-3342: Add thread counters to monitor plan fragment execution

Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change.

Change subject: IMPALA-3342: Add thread counters to monitor plan fragment execution
......................................................................


Patch Set 10: Code-Review+2

Hi Anuj, this didn't commit because you didn't carry the +2 forward to the rebased patch.  I assume you meant it to be committed? If so, can you do that?

-- 
To view, visit http://gerrit.cloudera.org:8080/4633
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifa88aa6f3371fa42d11ecc122f43c7d83623c300
Gerrit-PatchSet: 10
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: anujphadke <ap...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Yonghyun Hwang
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] Impala-3342: Adding thread counters to measure time spent during plan fragment execution

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.

Change subject: Impala-3342: Adding thread counters to measure time spent during plan fragment execution
......................................................................


Patch Set 5:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/4633/5//COMMIT_MSG
Commit Message:

Line 7: Impala-3342: Adding thread counters to measure time spent during plan
The formatting is still weird.


http://gerrit.cloudera.org:8080/#/c/4633/5/be/src/runtime/plan-fragment-executor.cc
File be/src/runtime/plan-fragment-executor.cc:

Line 347: <<<<<<< HEAD
Bad rebase?


-- 
To view, visit http://gerrit.cloudera.org:8080/4633
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifa88aa6f3371fa42d11ecc122f43c7d83623c300
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: anujphadke <ap...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Yonghyun Hwang
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3342: Add thread counters to monitor plan fragment execution

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.

Change subject: IMPALA-3342: Add thread counters to monitor plan fragment execution
......................................................................


Patch Set 6:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/4633/6//COMMIT_MSG
Commit Message:

Line 9: This change removes the use of total_cpu_timer which incorrectly
> Do you mean provide a snippet of the profile in the commit message?
Yes


http://gerrit.cloudera.org:8080/#/c/4633/6/be/src/runtime/runtime-state.cc
File be/src/runtime/runtime-state.cc:

Line 138
Why don't we create the thread counters here like before? This seems much cleaner rather than having PlanFragmentExecutor do it.


http://gerrit.cloudera.org:8080/#/c/4633/6/be/src/runtime/runtime-state.h
File be/src/runtime/runtime-state.h:

Line 242:   RuntimeProfile::ThreadCounters* plan_fragment_counters_;
> We access this object  variable in plan-fragment-executor.Hence I kept it p
The convention is that the trailing underscore means that the member is private. Generally all class members should be private and exposed if needed via accessor functions.

Sometimes we use friend classes if two classes are very tightly coupled by design.


-- 
To view, visit http://gerrit.cloudera.org:8080/4633
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifa88aa6f3371fa42d11ecc122f43c7d83623c300
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: anujphadke <ap...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Yonghyun Hwang
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3342: Add thread counters to monitor plan fragment execution

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.

Change subject: IMPALA-3342: Add thread counters to monitor plan fragment execution
......................................................................


Patch Set 6:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/4633/6//COMMIT_MSG
Commit Message:

Line 9: This change removes the use of total_cpu_timer which incorrectly
Can you provide an example of how the profile looks now? Otherwise the change looks good to me.


http://gerrit.cloudera.org:8080/#/c/4633/6/be/src/runtime/runtime-state.h
File be/src/runtime/runtime-state.h:

Line 242:   RuntimeProfile::ThreadCounters* plan_fragment_counters_;
This member variable should be private.


-- 
To view, visit http://gerrit.cloudera.org:8080/4633
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifa88aa6f3371fa42d11ecc122f43c7d83623c300
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: anujphadke <ap...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Yonghyun Hwang
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3342: Add thread counters to monitor plan fragment execution

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.

Change subject: IMPALA-3342: Add thread counters to monitor plan fragment execution
......................................................................


Patch Set 8: Code-Review+1

-- 
To view, visit http://gerrit.cloudera.org:8080/4633
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifa88aa6f3371fa42d11ecc122f43c7d83623c300
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: anujphadke <ap...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Yonghyun Hwang
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-3342: Add thread counters to monitor plan fragment execution

Posted by "Internal Jenkins (Code Review)" <ge...@cloudera.org>.
Internal Jenkins has posted comments on this change.

Change subject: IMPALA-3342: Add thread counters to monitor plan fragment execution
......................................................................


Patch Set 10: Verified+1

-- 
To view, visit http://gerrit.cloudera.org:8080/4633
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifa88aa6f3371fa42d11ecc122f43c7d83623c300
Gerrit-PatchSet: 10
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: anujphadke <ap...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Yonghyun Hwang
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] Impala-3342: Adding thread counters to measure time spent during plan fragment execution

Posted by "anujphadke (Code Review)" <ge...@cloudera.org>.
anujphadke has posted comments on this change.

Change subject: Impala-3342: Adding thread counters to measure time spent during plan fragment execution
......................................................................


Patch Set 2:

(6 comments)

http://gerrit.cloudera.org:8080/#/c/4633/2//COMMIT_MSG
Commit Message:

PS2, Line 7: Impala-3342 Adding thread counters to measure time spent during plan
           : fragment execution
> Please fix the formatting of the msg.
Done


PS2, Line 11: meausure
> measure
Done


PS2, Line 13: hdfs/kudu scanner and in a blocking join
> Why is this worth calling out? Doesn't this measure all exec nodes?
This change replaces every instance of the total_cpu_timer. Replacing this timer in those 2 places and adding the THREAD_COUNTERS which get aggregated in the plan-fragment-executor. 
Adding thread counters in every thread would bulk up the profile.


Line 14: 
> What does the profile look like? Would be helpful to see what the plan frag
Fragment F01:
      Instance c64650f62b67d849:ff790c3600000002 (host=anuj-OptiPlex-9020:22000):(Total: 31.008ms, non-child: 0.000ns, % non-child: 0.00%)
        Hdfs split stats (<volume id>:<# splits>/<split lengths>): 0:29/3.60 GB 
        MemoryUsage(500.000ms): 57.78 MB, 129.77 MB, 137.79 MB, 129.79 MB, 129.79 MB, 121.79 MB, 153.78 MB, 121.79 MB, 129.79 MB, 129.79 MB, 129.79 MB, 129.79 MB, 97.79 MB, 137.79 MB, 137.79 MB, 129.79 MB, 121.79 MB, 105.79 MB, 105.89 MB, 121.79 MB, 129.79 MB, 121.79 MB, 137.79 MB, 113.79 MB, 113.79 MB, 137.79 MB, 121.79 MB, 129.79 MB, 129.79 MB, 113.79 MB, 105.86 MB, 97.76 MB, 113.76 MB, 105.76 MB, 73.72 MB
        ThreadUsage(500.000ms): 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 5, 5, 5, 4
         - AverageThreadTokens: 5.86 
         - BloomFilterBytes: 0
         - PeakMemoryUsage: 169.83 MB (178079056)
         - PerHostPeakMemUsage: 1.17 GB (1256325736)
         - PlanFragmentThreadInvoluntaryContextSwitches: 4.32K (4321)
         - PlanFragmentThreadTotalWallClockTime: 1m42s
           - PlanFragmentThreadSysTime: 206.195ms
           - PlanFragmentThreadUserTime: 41s769ms
         - PlanFragmentThreadVoluntaryContextSwitches: 35.57K (35570)
         - PrepareTime: 30.293ms
         - RowsProduced: 30.00M (29999795)
         - TotalNetworkReceiveTime: 0.000ns
         - TotalNetworkSendTime: 2s396ms
         - TotalStorageWaitTime: 920.765ms
        CodeGen:(Total: 41.328ms, non-child: 41.328ms, % non-child: 100.00%)
           - CodegenTime: 518.571us
           - CompileTime: 3.161ms
           - LoadTime: 0.000ns
           - ModuleBitcodeSize: 1.90 MB (1996720)
           - NumFunctions: 9 (9)
           - NumInstructions: 113 (113)
           - OptimizationTime: 7.696ms
           - PrepareTime: 30.095ms
        DataStreamSender (dst_id=5):(Total: 16s230ms, non-child: 16s230ms, % non-child: 100.00%)
           - BytesSent: 428.18 MB (448979535)
           - NetworkThroughput(*): 236.47 MB/sec
           - OverallThroughput: 26.38 MB/sec


http://gerrit.cloudera.org:8080/#/c/4633/2/be/src/runtime/plan-fragment-executor.cc
File be/src/runtime/plan-fragment-executor.cc:

Line 213:       ADD_COUNTER(profile(), PER_HOST_PEAK_MEM_COUNTER, TUnit::BYTES);
> Let's create the new counter here along with the other ones so that the set
Done


http://gerrit.cloudera.org:8080/#/c/4633/2/be/src/runtime/runtime-state.h
File be/src/runtime/runtime-state.h:

Line 241:   ///Fragment thread counters
> The comment isn't too helpful. Maybe:
Done


-- 
To view, visit http://gerrit.cloudera.org:8080/4633
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifa88aa6f3371fa42d11ecc122f43c7d83623c300
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: anujphadke <ap...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Yonghyun Hwang
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3342: Add thread counters to monitor plan fragment execution

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.

Change subject: IMPALA-3342: Add thread counters to monitor plan fragment execution
......................................................................


Patch Set 8:

It would be good if Henry  could take a final look as well.

-- 
To view, visit http://gerrit.cloudera.org:8080/4633
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifa88aa6f3371fa42d11ecc122f43c7d83623c300
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: anujphadke <ap...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Yonghyun Hwang
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-3342: Add thread counters to monitor plan fragment execution

Posted by "anujphadke (Code Review)" <ge...@cloudera.org>.
anujphadke has uploaded a new patch set (#6).

Change subject: IMPALA-3342: Add thread counters to monitor plan fragment execution
......................................................................

IMPALA-3342: Add thread counters to monitor plan fragment execution

This change removes the use of total_cpu_timer which incorrectly
monitors the CPU time. Adding THREAD_COUNTERS to measure the user
and sys time in plan fragment execution. This also accounts for the
time spent in the hdfs/kudu scanner and in a blocking join.

Change-Id: Ifa88aa6f3371fa42d11ecc122f43c7d83623c300
---
M be/src/exec/blocking-join-node.cc
M be/src/exec/hdfs-scan-node.cc
M be/src/exec/kudu-scan-node.cc
M be/src/runtime/plan-fragment-executor.cc
M be/src/runtime/plan-fragment-executor.h
M be/src/runtime/runtime-state.cc
M be/src/runtime/runtime-state.h
7 files changed, 17 insertions(+), 26 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/33/4633/6
-- 
To view, visit http://gerrit.cloudera.org:8080/4633
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ifa88aa6f3371fa42d11ecc122f43c7d83623c300
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: anujphadke <ap...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Yonghyun Hwang
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>

[Impala-ASF-CR] IMPALA-3342: Add thread counters to monitor plan fragment execution

Posted by "anujphadke (Code Review)" <ge...@cloudera.org>.
anujphadke has uploaded a new patch set (#7).

Change subject: IMPALA-3342: Add thread counters to monitor plan fragment execution
......................................................................

IMPALA-3342: Add thread counters to monitor plan fragment execution

This change removes the use of total_cpu_timer which incorrectly
monitors the CPU time. Adding THREAD_COUNTERS to measure the user
and sys time in plan fragment execution. This also accounts for the
time spent in the hdfs/kudu scanner and in a blocking join.

Snippet of a query plan with the newly added PlanFragment
THREAD_COUNTERS:

    Fragment F00:
      Instance 22405f2d916ba9e6:41f8692c00000009
      .....
         - PerHostPeakMemUsage: 978.06 MB (1025568440)
         - PlanFragmentThreadInvoluntaryContextSwitches: 12.85K (12846)
         - PlanFragmentThreadTotalWallClockTime: 5m25s
           - PlanFragmentThreadSysTime: 147.673ms
           - PlanFragmentThreadUserTime: 20s710ms
         - PlanFragmentThreadVoluntaryContextSwitches: 13.43K (13427)

Change-Id: Ifa88aa6f3371fa42d11ecc122f43c7d83623c300
---
M be/src/exec/blocking-join-node.cc
M be/src/exec/hdfs-scan-node.cc
M be/src/exec/kudu-scan-node.cc
M be/src/runtime/plan-fragment-executor.cc
M be/src/runtime/plan-fragment-executor.h
M be/src/runtime/runtime-state.cc
M be/src/runtime/runtime-state.h
7 files changed, 19 insertions(+), 26 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/33/4633/7
-- 
To view, visit http://gerrit.cloudera.org:8080/4633
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ifa88aa6f3371fa42d11ecc122f43c7d83623c300
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: anujphadke <ap...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Yonghyun Hwang
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>

[Impala-ASF-CR] IMPALA-3342: Add thread counters to monitor plan fragment execution

Posted by "anujphadke (Code Review)" <ge...@cloudera.org>.
Hello Henry Robinson, Internal Jenkins, Dan Hecht, Tim Armstrong,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/4633

to look at the new patch set (#11).

Change subject: IMPALA-3342: Add thread counters to monitor plan fragment execution
......................................................................

IMPALA-3342: Add thread counters to monitor plan fragment execution

This change removes the use of total_cpu_timer which incorrectly
monitors the CPU time. Adding THREAD_COUNTERS to measure the user
and sys time in plan fragment execution. This also accounts for the
time spent in the hdfs/kudu scanner and in a blocking join.

Snippet of a query plan with the newly added PlanFragment
THREAD_COUNTERS:
      Instance 2b40b101e2626e7a:a3d8f2300000000
         - PeakMemoryUsage: 32.02 KB (32784)
         - PerHostPeakMemUsage: 430.52 MB (451431312)
         - RowsProduced: 1 (1)
         - TotalNetworkReceiveTime: 10s379ms
         - TotalNetworkSendTime: 0.000ns
         - TotalStorageWaitTime: 0.000ns
         - TotalWallClockTime: 10s577ms
           - SysTime: 8.000ms
           - UserTime: 8.000ms
         - VoluntaryContextSwitches: 80 (80)

Change-Id: Ifa88aa6f3371fa42d11ecc122f43c7d83623c300
---
M be/src/exec/blocking-join-node.cc
M be/src/exec/hdfs-scan-node.cc
M be/src/exec/kudu-scan-node.cc
M be/src/runtime/plan-fragment-executor.cc
M be/src/runtime/plan-fragment-executor.h
M be/src/runtime/runtime-state.cc
M be/src/runtime/runtime-state.h
7 files changed, 17 insertions(+), 25 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/33/4633/11
-- 
To view, visit http://gerrit.cloudera.org:8080/4633
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ifa88aa6f3371fa42d11ecc122f43c7d83623c300
Gerrit-PatchSet: 11
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: anujphadke <ap...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Yonghyun Hwang
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>

[Impala-ASF-CR] IMPALA-3342: Add thread counters to monitor plan fragment execution

Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has posted comments on this change.

Change subject: IMPALA-3342: Add thread counters to monitor plan fragment execution
......................................................................


Patch Set 11: Code-Review+2

Carrying Henry's +2 after rebase.

-- 
To view, visit http://gerrit.cloudera.org:8080/4633
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifa88aa6f3371fa42d11ecc122f43c7d83623c300
Gerrit-PatchSet: 11
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: anujphadke <ap...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Yonghyun Hwang
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] Impala-3342 Adding thread counters to measure time spent during plan fragment execution

Posted by "anujphadke (Code Review)" <ge...@cloudera.org>.
anujphadke has uploaded a new patch set (#4).

Change subject: Impala-3342 Adding thread counters to measure time spent during plan fragment execution
......................................................................

Impala-3342 Adding thread counters to measure time spent during plan
fragment execution

This change removes the use of total_cpu_timer which incorrectly
monitors the CPU time. Adding THREAD_COUNTERS to meausure the user
and sys time in plan fragment execution. This also accounts for the
time spent in the hdfs/kudu scanner and in a blocking join.

Change-Id: Ifa88aa6f3371fa42d11ecc122f43c7d83623c300
---
M be/src/exec/blocking-join-node.cc
M be/src/exec/hdfs-scan-node.cc
M be/src/exec/kudu-scan-node.cc
M be/src/runtime/plan-fragment-executor.cc
M be/src/runtime/plan-fragment-executor.h
M be/src/runtime/runtime-state.cc
M be/src/runtime/runtime-state.h
7 files changed, 17 insertions(+), 28 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/33/4633/4
-- 
To view, visit http://gerrit.cloudera.org:8080/4633
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ifa88aa6f3371fa42d11ecc122f43c7d83623c300
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: anujphadke <ap...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Yonghyun Hwang

[Impala-ASF-CR] IMPALA-3342: Add thread counters to monitor plan fragment execution

Posted by "anujphadke (Code Review)" <ge...@cloudera.org>.
anujphadke has posted comments on this change.

Change subject: IMPALA-3342: Add thread counters to monitor plan fragment execution
......................................................................


Patch Set 7:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/4633/6//COMMIT_MSG
Commit Message:

Line 9: This change removes the use of total_cpu_timer which incorrectly
> Yes
Done


http://gerrit.cloudera.org:8080/#/c/4633/6/be/src/runtime/runtime-state.cc
File be/src/runtime/runtime-state.cc:

Line 138
> Why don't we create the thread counters here like before? This seems much c
Done and I can access the private variable here too.


http://gerrit.cloudera.org:8080/#/c/4633/6/be/src/runtime/runtime-state.h
File be/src/runtime/runtime-state.h:

Line 242:   /// 'failed_allocation_size' is zero, nothing about the allocation size is logged.
> The convention is that the trailing underscore means that the member is pri
Done.
I start the timer in the runtime-state instead of in the plan-fragment-executor object.


-- 
To view, visit http://gerrit.cloudera.org:8080/4633
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifa88aa6f3371fa42d11ecc122f43c7d83623c300
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: anujphadke <ap...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Yonghyun Hwang
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3342: Add thread counters to monitor plan fragment execution

Posted by "anujphadke (Code Review)" <ge...@cloudera.org>.
Hello Tim Armstrong,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/4633

to look at the new patch set (#9).

Change subject: IMPALA-3342: Add thread counters to monitor plan fragment execution
......................................................................

IMPALA-3342: Add thread counters to monitor plan fragment execution

This change removes the use of total_cpu_timer which incorrectly
monitors the CPU time. Adding THREAD_COUNTERS to measure the user
and sys time in plan fragment execution. This also accounts for the
time spent in the hdfs/kudu scanner and in a blocking join.

Snippet of a query plan with the newly added PlanFragment
THREAD_COUNTERS:
      Instance 2b40b101e2626e7a:a3d8f2300000000
         - PeakMemoryUsage: 32.02 KB (32784)
         - PerHostPeakMemUsage: 430.52 MB (451431312)
         - RowsProduced: 1 (1)
         - TotalNetworkReceiveTime: 10s379ms
         - TotalNetworkSendTime: 0.000ns
         - TotalStorageWaitTime: 0.000ns
         - TotalWallClockTime: 10s577ms
           - SysTime: 8.000ms
           - UserTime: 8.000ms
         - VoluntaryContextSwitches: 80 (80)

Change-Id: Ifa88aa6f3371fa42d11ecc122f43c7d83623c300
---
M be/src/exec/blocking-join-node.cc
M be/src/exec/hdfs-scan-node.cc
M be/src/exec/kudu-scan-node.cc
M be/src/runtime/plan-fragment-executor.cc
M be/src/runtime/plan-fragment-executor.h
M be/src/runtime/runtime-state.cc
M be/src/runtime/runtime-state.h
7 files changed, 17 insertions(+), 26 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/33/4633/9
-- 
To view, visit http://gerrit.cloudera.org:8080/4633
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ifa88aa6f3371fa42d11ecc122f43c7d83623c300
Gerrit-PatchSet: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: anujphadke <ap...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Yonghyun Hwang
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>

[Impala-ASF-CR] IMPALA-3342: Add thread counters to monitor plan fragment execution

Posted by "anujphadke (Code Review)" <ge...@cloudera.org>.
anujphadke has posted comments on this change.

Change subject: IMPALA-3342: Add thread counters to monitor plan fragment execution
......................................................................


Patch Set 6:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/4633/6//COMMIT_MSG
Commit Message:

Line 9: This change removes the use of total_cpu_timer which incorrectly
> Can you provide an example of how the profile looks now? Otherwise the chan
Do you mean provide a snippet of the profile in the commit message?


http://gerrit.cloudera.org:8080/#/c/4633/6/be/src/runtime/runtime-state.h
File be/src/runtime/runtime-state.h:

Line 242:   RuntimeProfile::ThreadCounters* plan_fragment_counters_;
> This member variable should be private.
We access this object  variable in plan-fragment-executor.Hence I kept it public unlike in scan-node where ThreadCounters are kept protected and 
are accessed directly.
Should I add a setter too for this? I did not do it since its usage did not look consistent.


-- 
To view, visit http://gerrit.cloudera.org:8080/4633
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifa88aa6f3371fa42d11ecc122f43c7d83623c300
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: anujphadke <ap...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Yonghyun Hwang
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3342: Add thread counters to monitor plan fragment execution

Posted by "Internal Jenkins (Code Review)" <ge...@cloudera.org>.
Internal Jenkins has posted comments on this change.

Change subject: IMPALA-3342: Add thread counters to monitor plan fragment execution
......................................................................


Patch Set 11: Verified+1

-- 
To view, visit http://gerrit.cloudera.org:8080/4633
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifa88aa6f3371fa42d11ecc122f43c7d83623c300
Gerrit-PatchSet: 11
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: anujphadke <ap...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Yonghyun Hwang
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-3342: Add thread counters to monitor plan fragment execution

Posted by "anujphadke (Code Review)" <ge...@cloudera.org>.
anujphadke has posted comments on this change.

Change subject: IMPALA-3342: Add thread counters to monitor plan fragment execution
......................................................................


Patch Set 6:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4633/5/be/src/runtime/plan-fragment-executor.cc
File be/src/runtime/plan-fragment-executor.cc:

Line 347:     Status status;
> Bad rebase?
Done


-- 
To view, visit http://gerrit.cloudera.org:8080/4633
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifa88aa6f3371fa42d11ecc122f43c7d83623c300
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: anujphadke <ap...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Yonghyun Hwang
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3342: Adding new timer to accurately measure the TotalCpuTime

Posted by "Yonghyun Hwang (Code Review)" <ge...@cloudera.org>.
Yonghyun Hwang has posted comments on this change.

Change subject: IMPALA-3342: Adding new timer to accurately measure the TotalCpuTime
......................................................................


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4633/1/be/src/util/stopwatch.h
File be/src/util/stopwatch.h:

PS1, Line 237: int64_t utime_diff = (end.ru_stime.tv_sec - start_.ru_stime.tv_sec)  * 1000L * 1000L * 1000L
             :      + (end.ru_stime.tv_usec - start_.ru_stime.tv_usec);
just 2 simple questions. 

1. are we considering "system CPU time" only (excluding "user CPU time")?

2. by any chance, (someone resets resource usage?) can utime_diff be negative?


-- 
To view, visit http://gerrit.cloudera.org:8080/4633
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifa88aa6f3371fa42d11ecc122f43c7d83623c300
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: anujphadke <ap...@cloudera.com>
Gerrit-Reviewer: Yonghyun Hwang
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3342: Add thread counters to monitor plan fragment execution

Posted by "anujphadke (Code Review)" <ge...@cloudera.org>.
Hello Tim Armstrong,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/4633

to look at the new patch set (#8).

Change subject: IMPALA-3342: Add thread counters to monitor plan fragment execution
......................................................................

IMPALA-3342: Add thread counters to monitor plan fragment execution

This change removes the use of total_cpu_timer which incorrectly
monitors the CPU time. Adding THREAD_COUNTERS to measure the user
and sys time in plan fragment execution. This also accounts for the
time spent in the hdfs/kudu scanner and in a blocking join.

Snippet of a query plan with the newly added PlanFragment
THREAD_COUNTERS:

    Fragment F00:
      Instance 22405f2d916ba9e6:41f8692c00000009
      .....
         - PerHostPeakMemUsage: 978.06 MB (1025568440)
         - PlanFragmentThreadsInvoluntaryContextSwitches: 12.85K (12846)
         - PlanFragmentThreadsTotalWallClockTime: 5m25s
           - PlanFragmentThreadsSysTime: 147.673ms
           - PlanFragmentThreadsUserTime: 20s710ms
         - PlanFragmentThreadsVoluntaryContextSwitches: 13.43K (13427)

Change-Id: Ifa88aa6f3371fa42d11ecc122f43c7d83623c300
---
M be/src/exec/blocking-join-node.cc
M be/src/exec/hdfs-scan-node.cc
M be/src/exec/kudu-scan-node.cc
M be/src/runtime/plan-fragment-executor.cc
M be/src/runtime/plan-fragment-executor.h
M be/src/runtime/runtime-state.cc
M be/src/runtime/runtime-state.h
7 files changed, 19 insertions(+), 26 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/33/4633/8
-- 
To view, visit http://gerrit.cloudera.org:8080/4633
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ifa88aa6f3371fa42d11ecc122f43c7d83623c300
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: anujphadke <ap...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Yonghyun Hwang
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>

[Impala-ASF-CR] IMPALA-3342: Add thread counters to monitor plan fragment execution

Posted by "anujphadke (Code Review)" <ge...@cloudera.org>.
anujphadke has posted comments on this change.

Change subject: IMPALA-3342: Add thread counters to monitor plan fragment execution
......................................................................


Patch Set 8:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/4633/7/be/src/runtime/runtime-state.cc
File be/src/runtime/runtime-state.cc:

PS7, Line 137: PlanFragmentThread
> Let's call it "PlanFragmentThreads" to make it clear that there may be mult
Done


http://gerrit.cloudera.org:8080/#/c/4633/7/be/src/runtime/runtime-state.h
File be/src/runtime/runtime-state.h:

Line 365:   /// prohibit copies
> Let's group it with the other timers - above total_storage_wait_timer_
Done


-- 
To view, visit http://gerrit.cloudera.org:8080/4633
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifa88aa6f3371fa42d11ecc122f43c7d83623c300
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: anujphadke <ap...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Yonghyun Hwang
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] Impala-3342 Adding thread counters to measure time spent during plan fragment execution

Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has posted comments on this change.

Change subject: Impala-3342 Adding thread counters to measure time spent during plan fragment execution
......................................................................


Patch Set 2:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/4633/2//COMMIT_MSG
Commit Message:

PS2, Line 7: Impala-3342 Adding thread counters to measure time spent during plan
           : fragment execution
Please fix the formatting of the msg.


PS2, Line 11: meausure
measure


PS2, Line 13: hdfs/kudu scanner and in a blocking join
Why is this worth calling out? Doesn't this measure all exec nodes?


http://gerrit.cloudera.org:8080/#/c/4633/2/be/src/exec/hdfs-scan-node.cc
File be/src/exec/hdfs-scan-node.cc:

PS2, Line 360: SCOPED_THREAD_COUNTER_MEASUREMENT(runtime_state_->plan_fragment_counters());
Is this thread safe?


-- 
To view, visit http://gerrit.cloudera.org:8080/4633
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifa88aa6f3371fa42d11ecc122f43c7d83623c300
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: anujphadke <ap...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Yonghyun Hwang
Gerrit-HasComments: Yes

[Impala-ASF-CR] Impala-3342 Adding thread counters to measure time spent during plan fragment execution

Posted by "anujphadke (Code Review)" <ge...@cloudera.org>.
anujphadke has uploaded a new patch set (#3).

Change subject: Impala-3342 Adding thread counters to measure time spent during plan fragment execution
......................................................................

Impala-3342 Adding thread counters to measure time spent during plan
fragment execution

This change removes the use of total_cpu_timer which incorrectly
monitors the CPU time. Adding THREAD_COUNTERS to meausure the user
and sys time in plan fragment execution. This also accounts for the
time spent in the hdfs/kudu scanner and in a blocking join.

Change-Id: Ifa88aa6f3371fa42d11ecc122f43c7d83623c300
---
M be/src/exec/blocking-join-node.cc
M be/src/exec/hdfs-scan-node.cc
M be/src/exec/kudu-scan-node.cc
M be/src/runtime/plan-fragment-executor.cc
M be/src/runtime/plan-fragment-executor.h
M be/src/runtime/runtime-state.cc
M be/src/runtime/runtime-state.h
7 files changed, 17 insertions(+), 28 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/33/4633/3
-- 
To view, visit http://gerrit.cloudera.org:8080/4633
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ifa88aa6f3371fa42d11ecc122f43c7d83623c300
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: anujphadke <ap...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Yonghyun Hwang

[Impala-ASF-CR] IMPALA-3342: Add thread counters to monitor plan fragment execution

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.

Change subject: IMPALA-3342: Add thread counters to monitor plan fragment execution
......................................................................


Patch Set 7: Code-Review+1

(2 comments)

LGTM once you make a couple of small changes.

http://gerrit.cloudera.org:8080/#/c/4633/7/be/src/runtime/runtime-state.cc
File be/src/runtime/runtime-state.cc:

PS7, Line 137: PlanFragmentThread
Let's call it "PlanFragmentThreads" to make it clear that there may be multiple threads.


http://gerrit.cloudera.org:8080/#/c/4633/7/be/src/runtime/runtime-state.h
File be/src/runtime/runtime-state.h:

Line 365:   /// Total CPU utilization for all threads in this plan fragment.
Let's group it with the other timers - above total_storage_wait_timer_


-- 
To view, visit http://gerrit.cloudera.org:8080/4633
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifa88aa6f3371fa42d11ecc122f43c7d83623c300
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: anujphadke <ap...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Yonghyun Hwang
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3342: Adding new timer to accurately measure the TotalCpuTime

Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has posted comments on this change.

Change subject: IMPALA-3342: Adding new timer to accurately measure the TotalCpuTime
......................................................................


Patch Set 1:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/4633/1//COMMIT_MSG
Commit Message:

PS1, Line 10: correctly
incorrectly


http://gerrit.cloudera.org:8080/#/c/4633/1/be/src/util/stopwatch.h
File be/src/util/stopwatch.h:

Line 28: #include<sys/resource.h>
space after include - please run git-clang-format over your patches before submitting.


PS1, Line 190: CpuTimeStopWatch
Do you think we actually need a stopwatch here? What about a method that just returns the ns computed from getrusage()? Then the PFE can just get the time at the start of the computation, and then compute the delta when the fragment is complete.


PS1, Line 190: class
class needs a comment


PS1, Line 225: end
Why does this need to be a member?


-- 
To view, visit http://gerrit.cloudera.org:8080/4633
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifa88aa6f3371fa42d11ecc122f43c7d83623c300
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: anujphadke <ap...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Yonghyun Hwang
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3342: Add thread counters to monitor plan fragment execution

Posted by "Internal Jenkins (Code Review)" <ge...@cloudera.org>.
Internal Jenkins has submitted this change and it was merged.

Change subject: IMPALA-3342: Add thread counters to monitor plan fragment execution
......................................................................


IMPALA-3342: Add thread counters to monitor plan fragment execution

This change removes the use of total_cpu_timer which incorrectly
monitors the CPU time. Adding THREAD_COUNTERS to measure the user
and sys time in plan fragment execution. This also accounts for the
time spent in the hdfs/kudu scanner and in a blocking join.

Snippet of a query plan with the newly added PlanFragment
THREAD_COUNTERS:
      Instance 2b40b101e2626e7a:a3d8f2300000000
         - PeakMemoryUsage: 32.02 KB (32784)
         - PerHostPeakMemUsage: 430.52 MB (451431312)
         - RowsProduced: 1 (1)
         - TotalNetworkReceiveTime: 10s379ms
         - TotalNetworkSendTime: 0.000ns
         - TotalStorageWaitTime: 0.000ns
         - TotalWallClockTime: 10s577ms
           - SysTime: 8.000ms
           - UserTime: 8.000ms
         - VoluntaryContextSwitches: 80 (80)

Change-Id: Ifa88aa6f3371fa42d11ecc122f43c7d83623c300
Reviewed-on: http://gerrit.cloudera.org:8080/4633
Reviewed-by: Bharath Vissapragada <bh...@cloudera.com>
Tested-by: Internal Jenkins
---
M be/src/exec/blocking-join-node.cc
M be/src/exec/hdfs-scan-node.cc
M be/src/exec/kudu-scan-node.cc
M be/src/runtime/plan-fragment-executor.cc
M be/src/runtime/plan-fragment-executor.h
M be/src/runtime/runtime-state.cc
M be/src/runtime/runtime-state.h
7 files changed, 17 insertions(+), 25 deletions(-)

Approvals:
  Bharath Vissapragada: Looks good to me, approved
  Internal Jenkins: Verified



-- 
To view, visit http://gerrit.cloudera.org:8080/4633
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: Ifa88aa6f3371fa42d11ecc122f43c7d83623c300
Gerrit-PatchSet: 12
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: anujphadke <ap...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Yonghyun Hwang
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>

[Impala-ASF-CR] IMPALA-3342: Add thread counters to monitor plan fragment execution

Posted by "anujphadke (Code Review)" <ge...@cloudera.org>.
anujphadke has posted comments on this change.

Change subject: IMPALA-3342: Add thread counters to monitor plan fragment execution
......................................................................


Patch Set 9:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/4633/8/be/src/runtime/plan-fragment-executor.cc
File be/src/runtime/plan-fragment-executor.cc:

PS8, Line 42: #include "runtime/runtime-state.
> what do you need this for?
This came from an older patch. Removed.


http://gerrit.cloudera.org:8080/#/c/4633/8/be/src/runtime/runtime-state.cc
File be/src/runtime/runtime-state.cc:

PS8, Line 137: rage_wait_timer_ = 
> Does it work if you pass an empty string? I think "PlanFragmentThreads" is 
Done


http://gerrit.cloudera.org:8080/#/c/4633/8/be/src/runtime/runtime-state.h
File be/src/runtime/runtime-state.h:

PS8, Line 323: total_thread_statistics
> how about calling this total_thread_statistics_ or similar?
Done


-- 
To view, visit http://gerrit.cloudera.org:8080/4633
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifa88aa6f3371fa42d11ecc122f43c7d83623c300
Gerrit-PatchSet: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: anujphadke <ap...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Yonghyun Hwang
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3342: Add thread counters to monitor plan fragment execution

Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has posted comments on this change.

Change subject: IMPALA-3342: Add thread counters to monitor plan fragment execution
......................................................................


Patch Set 8:

(3 comments)

Couple of naming questions, otherwise looks good.

http://gerrit.cloudera.org:8080/#/c/4633/8/be/src/runtime/plan-fragment-executor.cc
File be/src/runtime/plan-fragment-executor.cc:

PS8, Line 42: #include "runtime/mem-tracker.h"
what do you need this for?


http://gerrit.cloudera.org:8080/#/c/4633/8/be/src/runtime/runtime-state.cc
File be/src/runtime/runtime-state.cc:

PS8, Line 137: PlanFragmentThreads
Does it work if you pass an empty string? I think "PlanFragmentThreads" is redundant, given the suffixes attached to all the counters.


http://gerrit.cloudera.org:8080/#/c/4633/8/be/src/runtime/runtime-state.h
File be/src/runtime/runtime-state.h:

PS8, Line 323: plan_fragment_counters_
how about calling this total_thread_statistics_ or similar?

plan_fragment_counters_ doesn't tell me anything about what it contains.


-- 
To view, visit http://gerrit.cloudera.org:8080/4633
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifa88aa6f3371fa42d11ecc122f43c7d83623c300
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: anujphadke <ap...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Yonghyun Hwang
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-HasComments: Yes