impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Henry Robinson (Code Review)" <ger...@cloudera.org>
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:

(11 comments)

Thanks for adding the tests.

http://gerrit.cloudera.org:8080/#/c/2578/4/be/src/runtime/data-stream-sender.cc
File be/src/runtime/data-stream-sender.cc:

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.


http://gerrit.cloudera.org:8080/#/c/2578/4/be/src/util/runtime-profile-test.cc
File be/src/util/runtime-profile-test.cc:

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.


http://gerrit.cloudera.org:8080/#/c/2578/4/be/src/util/runtime-profile.h
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?


http://gerrit.cloudera.org:8080/#/c/2578/4/be/src/util/stopwatch.h
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
get:

  Start()
  Stop()
  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 data-stream-sender.cc for whether you need this method.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I9c530b2056d852c8bcac6263e9e6b1a6bede1047
Gerrit-PatchSet: 4
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Juan Yu <jyu@cloudera.com>
Gerrit-Reviewer: Henry Robinson <henry@cloudera.com>
Gerrit-Reviewer: Juan Yu <jyu@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sailesh@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message