You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Tim Armstrong (Code Review)" <ge...@cloudera.org> on 2019/09/24 21:18:11 UTC
[Impala-ASF-CR] IMPALA-2312: simplify stopwatch ElapsedTime()
Tim Armstrong has uploaded this change for review. ( http://gerrit.cloudera.org:8080/14293
Change subject: IMPALA-2312: simplify stopwatch ElapsedTime()
......................................................................
IMPALA-2312: simplify stopwatch ElapsedTime()
The stopwatches had two subtly different ElapsedTime()
interfaces, one that included previous running time
and one that didn't. They do not appear to be both
needed. I looked through all the uses and in all
cases using the old TotalElapsedTime() in place of
ElapsedTime() is equivalent, either because the
stopwatch is stopped when ElapsedTime() is called
or because Stop() is never called.
Change-Id: I004833681248de65ce709fc746269686507aba54
---
M be/src/exec/blocking-join-node.cc
M be/src/util/stopwatch.h
2 files changed, 19 insertions(+), 25 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/93/14293/1
--
To view, visit http://gerrit.cloudera.org:8080/14293
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I004833681248de65ce709fc746269686507aba54
Gerrit-Change-Number: 14293
Gerrit-PatchSet: 1
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-2312: simplify stopwatch ElapsedTime()
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14293 )
Change subject: IMPALA-2312: simplify stopwatch ElapsedTime()
......................................................................
Patch Set 2:
Carry Csaba's +2
--
To view, visit http://gerrit.cloudera.org:8080/14293
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I004833681248de65ce709fc746269686507aba54
Gerrit-Change-Number: 14293
Gerrit-PatchSet: 2
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 26 Sep 2019 17:29:19 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-2312: simplify stopwatch ElapsedTime()
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/14293 )
Change subject: IMPALA-2312: simplify stopwatch ElapsedTime()
......................................................................
IMPALA-2312: simplify stopwatch ElapsedTime()
The stopwatches had two subtly different ElapsedTime()
interfaces, one that included previous running time
and one that didn't. They do not appear to be both
needed. I looked through all the uses and in all
cases using the old TotalElapsedTime() in place of
ElapsedTime() is equivalent, either because the
stopwatch is stopped when ElapsedTime() is called
or because Stop() is never called.
Change-Id: I004833681248de65ce709fc746269686507aba54
Reviewed-on: http://gerrit.cloudera.org:8080/14293
Reviewed-by: Tim Armstrong <ta...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M be/src/exec/blocking-join-node.cc
M be/src/util/stopwatch.h
2 files changed, 21 insertions(+), 29 deletions(-)
Approvals:
Tim Armstrong: Looks good to me, approved
Impala Public Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/14293
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I004833681248de65ce709fc746269686507aba54
Gerrit-Change-Number: 14293
Gerrit-PatchSet: 4
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-2312: simplify stopwatch ElapsedTime()
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14293 )
Change subject: IMPALA-2312: simplify stopwatch ElapsedTime()
......................................................................
Patch Set 3: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/14293
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I004833681248de65ce709fc746269686507aba54
Gerrit-Change-Number: 14293
Gerrit-PatchSet: 3
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 26 Sep 2019 21:43:42 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-2312: simplify stopwatch ElapsedTime()
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14293 )
Change subject: IMPALA-2312: simplify stopwatch ElapsedTime()
......................................................................
Patch Set 1:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/4632/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/14293
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I004833681248de65ce709fc746269686507aba54
Gerrit-Change-Number: 14293
Gerrit-PatchSet: 1
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Tue, 24 Sep 2019 21:59:19 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-2312: simplify stopwatch ElapsedTime()
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14293 )
Change subject: IMPALA-2312: simplify stopwatch ElapsedTime()
......................................................................
Patch Set 3:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/5006/ DRY_RUN=false
--
To view, visit http://gerrit.cloudera.org:8080/14293
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I004833681248de65ce709fc746269686507aba54
Gerrit-Change-Number: 14293
Gerrit-PatchSet: 3
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 26 Sep 2019 17:29:33 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-2312: simplify stopwatch ElapsedTime()
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14293 )
Change subject: IMPALA-2312: simplify stopwatch ElapsedTime()
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/14293/1/be/src/util/stopwatch.h
File be/src/util/stopwatch.h:
http://gerrit.cloudera.org:8080/#/c/14293/1/be/src/util/stopwatch.h@116
PS1, Line 116: void Stop() {
: total_time_ += RunningTime();
: running_ = false;
: }
> nit, consistancy: this and StopWatch's stop seem to to exactly the same, as
Done
--
To view, visit http://gerrit.cloudera.org:8080/14293
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I004833681248de65ce709fc746269686507aba54
Gerrit-Change-Number: 14293
Gerrit-PatchSet: 1
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 26 Sep 2019 17:29:12 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-2312: simplify stopwatch ElapsedTime()
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/14293 )
Change subject: IMPALA-2312: simplify stopwatch ElapsedTime()
......................................................................
Patch Set 1: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/14293/1/be/src/util/stopwatch.h
File be/src/util/stopwatch.h:
http://gerrit.cloudera.org:8080/#/c/14293/1/be/src/util/stopwatch.h@116
PS1, Line 116: void Stop() {
: total_time_ += RunningTime();
: running_ = false;
: }
nit, consistancy: this and StopWatch's stop seem to to exactly the same, as RunningTime() returns 0 is running_ is false. I think that it would be better if both would be the same, as people would not start looking for differences :)
--
To view, visit http://gerrit.cloudera.org:8080/14293
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I004833681248de65ce709fc746269686507aba54
Gerrit-Change-Number: 14293
Gerrit-PatchSet: 1
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Thu, 26 Sep 2019 14:15:06 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-2312: simplify stopwatch ElapsedTime()
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14293 )
Change subject: IMPALA-2312: simplify stopwatch ElapsedTime()
......................................................................
Patch Set 2:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/4650/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/14293
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I004833681248de65ce709fc746269686507aba54
Gerrit-Change-Number: 14293
Gerrit-PatchSet: 2
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 26 Sep 2019 18:12:45 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-2312: simplify stopwatch ElapsedTime()
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14293 )
Change subject: IMPALA-2312: simplify stopwatch ElapsedTime()
......................................................................
Patch Set 3: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/14293
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I004833681248de65ce709fc746269686507aba54
Gerrit-Change-Number: 14293
Gerrit-PatchSet: 3
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 26 Sep 2019 17:34:34 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-2312: simplify stopwatch ElapsedTime()
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Hello Csaba Ringhofer, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14293
to look at the new patch set (#2).
Change subject: IMPALA-2312: simplify stopwatch ElapsedTime()
......................................................................
IMPALA-2312: simplify stopwatch ElapsedTime()
The stopwatches had two subtly different ElapsedTime()
interfaces, one that included previous running time
and one that didn't. They do not appear to be both
needed. I looked through all the uses and in all
cases using the old TotalElapsedTime() in place of
ElapsedTime() is equivalent, either because the
stopwatch is stopped when ElapsedTime() is called
or because Stop() is never called.
Change-Id: I004833681248de65ce709fc746269686507aba54
---
M be/src/exec/blocking-join-node.cc
M be/src/util/stopwatch.h
2 files changed, 21 insertions(+), 29 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/93/14293/2
--
To view, visit http://gerrit.cloudera.org:8080/14293
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I004833681248de65ce709fc746269686507aba54
Gerrit-Change-Number: 14293
Gerrit-PatchSet: 2
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>