impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Henry Robinson (Code Review)" <>
Subject [Impala-CR](cdh5-trunk) IMPALA-2076: Correct execution time tracking for DataStreamSender.
Date Thu, 14 Apr 2016 22:29:36 GMT
Henry Robinson has posted comments on this change.

Change subject: IMPALA-2076: Correct execution time tracking for DataStreamSender.

Patch Set 4:


Thanks for adding the tests.
File be/src/runtime/

Line 209:   COUNTER_ADD(parent_->profile_->total_time_counter(),
I'm not totally sure if this does the right thing - presumably you want to add the wall-clock
time for all sender threads together?

Maybe it would be simpler just to run a stopwatch here to measure the wallclock time around
the RPC call, and add that in.
File be/src/util/

Line 550:       delete thread_handle;
how does this work if thread_handle is NULL (see line 584)?

Line 557:     while(!worker->done) {
space before (

Line 577:   void StopWorkers(int index) {
needs a comment. maybe call 'index' something like 'exclude_idx' and default it to -1?

Line 580: index >= 0
how would index be < 0?

Line 589:   static const int MAX_NUM_THREADS = 5;
why have this, rather than just initialize workers_[] in StartWorkers?

Line 608: void ValidateLapTime(TimerCounterTest& timer, int64_t expected_value) {
missing const?

Line 638: int running_time = 0;
rather than tracking running_time per sleep-period, it might be better to use wall-clock time
where possible. That way, if the thread was forced to sleep by the OS for an extra 100ms before
calling ValidateTimerValue(), there's a better chance that the test will still pass.

Line 659:   tester.StartWorkers(2, 0);
suggest adding a sleep before this to show that the time between working periods wasn't included
in the timer.
File be/src/util/runtime-profile.h:

Line 379: If a thread is already running, the following thread won't reset the stop watch.
this is confusing - what's the following thread?
File be/src/util/stopwatch.h:

Line 208: /// Returns running time since last time LapTime() is called and reset lap_time_
        :   /// back to 0.
this doesn't seem to match the implementation. If we call LapTime() twice in succession, we'd

  LapTime() -> e.g. 100ms
  LapTime() -> 0ms

Usually the laptime means you'd do something like this:

  LapTime() {
    uint64_t now = msw_.TotalElapsedTime();
    uint64_t lap_duration = now - last_lap_start_;
    last_lap_start_ = now;
    return lap_duration;

See my comment in for whether you need this method.

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I9c530b2056d852c8bcac6263e9e6b1a6bede1047
Gerrit-PatchSet: 4
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Juan Yu <>
Gerrit-Reviewer: Henry Robinson <>
Gerrit-Reviewer: Juan Yu <>
Gerrit-Reviewer: Sailesh Mukil <>
Gerrit-HasComments: Yes

View raw message