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 2018/01/29 23:55:40 UTC
[Impala-ASF-CR] IMPALA-6450: loosen DCHECK in EventSequence::Start()
Tim Armstrong has uploaded this change for review. ( http://gerrit.cloudera.org:8080/9155
Change subject: IMPALA-6450: loosen DCHECK in EventSequence::Start()
......................................................................
IMPALA-6450: loosen DCHECK in EventSequence::Start()
It looks like this newly-added DCHECK is being hit because of the same
underlying issue as IMPALA-4631. This patch loosens the DCHECK to accept
time going backward 1 tick, the same as the original workaround for
IMPALA-4631.
Change-Id: I62317149cb8428f7d29e945a538f6cc8dd45342f
---
M be/src/util/runtime-profile-counters.h
1 file changed, 4 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/55/9155/1
--
To view, visit http://gerrit.cloudera.org:8080/9155
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I62317149cb8428f7d29e945a538f6cc8dd45342f
Gerrit-Change-Number: 9155
Gerrit-PatchSet: 1
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-6450: fix EventSequence::Start()
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/9155 )
Change subject: IMPALA-6450: fix EventSequence::Start()
......................................................................
Patch Set 3:
Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/1838/
--
To view, visit http://gerrit.cloudera.org:8080/9155
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I62317149cb8428f7d29e945a538f6cc8dd45342f
Gerrit-Change-Number: 9155
Gerrit-PatchSet: 3
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-Comment-Date: Wed, 31 Jan 2018 10:06:44 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6450: fix EventSequence::Start()
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/9155 )
Change subject: IMPALA-6450: fix EventSequence::Start()
......................................................................
Patch Set 4:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1843/
--
To view, visit http://gerrit.cloudera.org:8080/9155
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I62317149cb8428f7d29e945a538f6cc8dd45342f
Gerrit-Change-Number: 9155
Gerrit-PatchSet: 4
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-Comment-Date: Wed, 31 Jan 2018 22:11:49 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6450: fix EventSequence::Start()
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker has posted comments on this change. ( http://gerrit.cloudera.org:8080/9155 )
Change subject: IMPALA-6450: fix EventSequence::Start()
......................................................................
Patch Set 3: Code-Review+1
Thank you for fixing this!
--
To view, visit http://gerrit.cloudera.org:8080/9155
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I62317149cb8428f7d29e945a538f6cc8dd45342f
Gerrit-Change-Number: 9155
Gerrit-PatchSet: 3
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-Comment-Date: Tue, 30 Jan 2018 19:30:40 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6450: fix EventSequence::Start()
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/9155 )
Change subject: IMPALA-6450: fix EventSequence::Start()
......................................................................
Patch Set 3: Code-Review+2
Also not for this patch, but Lars, is there some test we are missing that would have caught the offset_ omission?
--
To view, visit http://gerrit.cloudera.org:8080/9155
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I62317149cb8428f7d29e945a538f6cc8dd45342f
Gerrit-Change-Number: 9155
Gerrit-PatchSet: 3
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-Comment-Date: Tue, 30 Jan 2018 21:13:44 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6450: fix EventSequence::Start()
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker has posted comments on this change. ( http://gerrit.cloudera.org:8080/9155 )
Change subject: IMPALA-6450: fix EventSequence::Start()
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/9155/2/be/src/util/runtime-profile-counters.h
File be/src/util/runtime-profile-counters.h:
http://gerrit.cloudera.org:8080/#/c/9155/2/be/src/util/runtime-profile-counters.h@340
PS2, Line 340: /// 'events_'. The caller must make sure that 'timestamps' is sorted.
Should we amend this comment to mention that the function does not consider offset_?
--
To view, visit http://gerrit.cloudera.org:8080/9155
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I62317149cb8428f7d29e945a538f6cc8dd45342f
Gerrit-Change-Number: 9155
Gerrit-PatchSet: 2
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-Comment-Date: Tue, 30 Jan 2018 02:10:40 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6450: loosen DCHECK in EventSequence::Start()
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/9155 )
Change subject: IMPALA-6450: loosen DCHECK in EventSequence::Start()
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/9155/1/be/src/util/runtime-profile-counters.h
File be/src/util/runtime-profile-counters.h:
http://gerrit.cloudera.org:8080/#/c/9155/1/be/src/util/runtime-profile-counters.h@308
PS1, Line 308: DCHECK_GE(offset_, -1);
> does anything bad happen when offset_ == -1? I.e. the fix of the DCHECK is
I can't even find where offset_ is used...
--
To view, visit http://gerrit.cloudera.org:8080/9155
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I62317149cb8428f7d29e945a538f6cc8dd45342f
Gerrit-Change-Number: 9155
Gerrit-PatchSet: 1
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-Comment-Date: Tue, 30 Jan 2018 00:23:29 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6450: fix EventSequence::Start()
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Hello Lars Volker, anujphadke, Dan Hecht,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9155
to look at the new patch set (#2).
Change subject: IMPALA-6450: fix EventSequence::Start()
......................................................................
IMPALA-6450: fix EventSequence::Start()
It looks like this newly-added DCHECK is being hit because of the same
underlying issue as IMPALA-4631. This patch loosens the DCHECK to accept
time going backward 1 tick, the same as the original workaround for
IMPALA-4631.
'offset_' also isn't being used, so fix that too.
Change-Id: I62317149cb8428f7d29e945a538f6cc8dd45342f
---
M be/src/util/runtime-profile-counters.h
1 file changed, 5 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/55/9155/2
--
To view, visit http://gerrit.cloudera.org:8080/9155
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I62317149cb8428f7d29e945a538f6cc8dd45342f
Gerrit-Change-Number: 9155
Gerrit-PatchSet: 2
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
[Impala-ASF-CR] IMPALA-6450: loosen DCHECK in EventSequence::Start()
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/9155 )
Change subject: IMPALA-6450: loosen DCHECK in EventSequence::Start()
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/9155/1/be/src/util/runtime-profile-counters.h
File be/src/util/runtime-profile-counters.h:
http://gerrit.cloudera.org:8080/#/c/9155/1/be/src/util/runtime-profile-counters.h@308
PS1, Line 308: DCHECK_GE(offset_, -1);
does anything bad happen when offset_ == -1? I.e. the fix of the DCHECK is fine, but do we also need to clamp offset_ to 0?
--
To view, visit http://gerrit.cloudera.org:8080/9155
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I62317149cb8428f7d29e945a538f6cc8dd45342f
Gerrit-Change-Number: 9155
Gerrit-PatchSet: 1
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-Comment-Date: Tue, 30 Jan 2018 00:18:17 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6450: loosen DCHECK in EventSequence::Start()
Posted by "anujphadke (Code Review)" <ge...@cloudera.org>.
anujphadke has posted comments on this change. ( http://gerrit.cloudera.org:8080/9155 )
Change subject: IMPALA-6450: loosen DCHECK in EventSequence::Start()
......................................................................
Patch Set 1: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/9155
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I62317149cb8428f7d29e945a538f6cc8dd45342f
Gerrit-Change-Number: 9155
Gerrit-PatchSet: 1
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-Comment-Date: Tue, 30 Jan 2018 00:12:16 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6450: fix EventSequence::Start()
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/9155 )
Change subject: IMPALA-6450: fix EventSequence::Start()
......................................................................
Patch Set 3:
Hit IMPALA-6455
--
To view, visit http://gerrit.cloudera.org:8080/9155
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I62317149cb8428f7d29e945a538f6cc8dd45342f
Gerrit-Change-Number: 9155
Gerrit-PatchSet: 3
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-Comment-Date: Wed, 31 Jan 2018 01:08:15 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6450: fix EventSequence::Start()
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/9155 )
Change subject: IMPALA-6450: fix EventSequence::Start()
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/9155/1/be/src/util/runtime-profile-counters.h
File be/src/util/runtime-profile-counters.h:
http://gerrit.cloudera.org:8080/#/c/9155/1/be/src/util/runtime-profile-counters.h@308
PS1, Line 308: DCHECK_GE(offset_, -1);
> Uh, I seem to have missed that. Tim, let me know if you'd like to fix it, o
Done
--
To view, visit http://gerrit.cloudera.org:8080/9155
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I62317149cb8428f7d29e945a538f6cc8dd45342f
Gerrit-Change-Number: 9155
Gerrit-PatchSet: 1
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-Comment-Date: Tue, 30 Jan 2018 01:56:19 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6450: fix EventSequence::Start()
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/9155 )
Change subject: IMPALA-6450: fix EventSequence::Start()
......................................................................
Patch Set 3:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1831/
--
To view, visit http://gerrit.cloudera.org:8080/9155
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I62317149cb8428f7d29e945a538f6cc8dd45342f
Gerrit-Change-Number: 9155
Gerrit-PatchSet: 3
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-Comment-Date: Tue, 30 Jan 2018 21:14:18 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6450: fix EventSequence::Start()
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/9155 )
Change subject: IMPALA-6450: fix EventSequence::Start()
......................................................................
Patch Set 4: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/9155
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I62317149cb8428f7d29e945a538f6cc8dd45342f
Gerrit-Change-Number: 9155
Gerrit-PatchSet: 4
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-Comment-Date: Wed, 31 Jan 2018 22:11:43 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6450: fix EventSequence::Start()
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/9155 )
Change subject: IMPALA-6450: fix EventSequence::Start()
......................................................................
Patch Set 3:
Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/1835/
--
To view, visit http://gerrit.cloudera.org:8080/9155
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I62317149cb8428f7d29e945a538f6cc8dd45342f
Gerrit-Change-Number: 9155
Gerrit-PatchSet: 3
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-Comment-Date: Wed, 31 Jan 2018 04:43:23 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6450: fix EventSequence::Start()
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/9155 )
Change subject: IMPALA-6450: fix EventSequence::Start()
......................................................................
Patch Set 3: Verified-1
Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/1831/
--
To view, visit http://gerrit.cloudera.org:8080/9155
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I62317149cb8428f7d29e945a538f6cc8dd45342f
Gerrit-Change-Number: 9155
Gerrit-PatchSet: 3
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-Comment-Date: Wed, 31 Jan 2018 00:56:28 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6450: fix EventSequence::Start()
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/9155 )
Change subject: IMPALA-6450: fix EventSequence::Start()
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/9155/2/be/src/util/runtime-profile-counters.h
File be/src/util/runtime-profile-counters.h:
http://gerrit.cloudera.org:8080/#/c/9155/2/be/src/util/runtime-profile-counters.h@304
PS2, Line 304: offset_ = MonotonicStopWatch::Now() - start_time_ns;
: // TODO: IMPALA-4631: Occasionally we see MonotonicStopWatch::Now() return
: // (start_time_ns - 1), even though 'start_time_ns' was obtained using
: // MonotonicStopWatch::Now().
: DCHECK_GE(offset_, -1);
: sw_.Start();
Not for this change, but I had to dig around to understand what this offset_ was really about (and look at the change that added it).
Lars, I wonder if this would be clearer if either:
a) We expose a sw_.StartTime(), and then offset can be computed from that, to make it more obvious that we're trying to retroactively start sw_.
b) We make a way to set the start_ time in sw_ directly (i.e. pass start_time_ns to a new method sw_.StartSynchronized() that starts the clock using the given time, or something). And then we don't need this offset at at.
--
To view, visit http://gerrit.cloudera.org:8080/9155
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I62317149cb8428f7d29e945a538f6cc8dd45342f
Gerrit-Change-Number: 9155
Gerrit-PatchSet: 2
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-Comment-Date: Tue, 30 Jan 2018 17:37:42 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6450: loosen DCHECK in EventSequence::Start()
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/9155 )
Change subject: IMPALA-6450: loosen DCHECK in EventSequence::Start()
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/9155/1/be/src/util/runtime-profile-counters.h
File be/src/util/runtime-profile-counters.h:
http://gerrit.cloudera.org:8080/#/c/9155/1/be/src/util/runtime-profile-counters.h@308
PS1, Line 308: DCHECK_GE(offset_, -1);
> I can't even find where offset_ is used...
That's a good point. The intent was to add it to sw_.ElapsedTime() in MarkEvent() but that code is obviously not there.
--
To view, visit http://gerrit.cloudera.org:8080/9155
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I62317149cb8428f7d29e945a538f6cc8dd45342f
Gerrit-Change-Number: 9155
Gerrit-PatchSet: 1
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-Comment-Date: Tue, 30 Jan 2018 01:09:46 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6450: fix EventSequence::Start()
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/9155 )
Change subject: IMPALA-6450: fix EventSequence::Start()
......................................................................
IMPALA-6450: fix EventSequence::Start()
It looks like this newly-added DCHECK is being hit because of the same
underlying issue as IMPALA-4631. This patch loosens the DCHECK to accept
time going backward 1 tick, the same as the original workaround for
IMPALA-4631.
'offset_' also isn't being used, so fix that too.
Change-Id: I62317149cb8428f7d29e945a538f6cc8dd45342f
Reviewed-on: http://gerrit.cloudera.org:8080/9155
Reviewed-by: Tim Armstrong <ta...@cloudera.com>
Tested-by: Impala Public Jenkins
---
M be/src/util/runtime-profile-counters.h
1 file changed, 7 insertions(+), 3 deletions(-)
Approvals:
Tim Armstrong: Looks good to me, approved
Impala Public Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/9155
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I62317149cb8428f7d29e945a538f6cc8dd45342f
Gerrit-Change-Number: 9155
Gerrit-PatchSet: 5
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
[Impala-ASF-CR] IMPALA-6450: fix EventSequence::Start()
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/9155 )
Change subject: IMPALA-6450: fix EventSequence::Start()
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/9155/2/be/src/util/runtime-profile-counters.h
File be/src/util/runtime-profile-counters.h:
http://gerrit.cloudera.org:8080/#/c/9155/2/be/src/util/runtime-profile-counters.h@340
PS2, Line 340: /// 'events_'. The caller must make sure that 'timestamps' is sorted.
> Should we amend this comment to mention that the function does not consider
Done
--
To view, visit http://gerrit.cloudera.org:8080/9155
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I62317149cb8428f7d29e945a538f6cc8dd45342f
Gerrit-Change-Number: 9155
Gerrit-PatchSet: 2
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-Comment-Date: Tue, 30 Jan 2018 19:22:02 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6450: fix EventSequence::Start()
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/9155 )
Change subject: IMPALA-6450: fix EventSequence::Start()
......................................................................
Patch Set 3:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1835/
--
To view, visit http://gerrit.cloudera.org:8080/9155
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I62317149cb8428f7d29e945a538f6cc8dd45342f
Gerrit-Change-Number: 9155
Gerrit-PatchSet: 3
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-Comment-Date: Wed, 31 Jan 2018 01:03:46 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6450: fix EventSequence::Start()
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Hello Lars Volker, anujphadke, Dan Hecht,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9155
to look at the new patch set (#3).
Change subject: IMPALA-6450: fix EventSequence::Start()
......................................................................
IMPALA-6450: fix EventSequence::Start()
It looks like this newly-added DCHECK is being hit because of the same
underlying issue as IMPALA-4631. This patch loosens the DCHECK to accept
time going backward 1 tick, the same as the original workaround for
IMPALA-4631.
'offset_' also isn't being used, so fix that too.
Change-Id: I62317149cb8428f7d29e945a538f6cc8dd45342f
---
M be/src/util/runtime-profile-counters.h
1 file changed, 7 insertions(+), 3 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/55/9155/3
--
To view, visit http://gerrit.cloudera.org:8080/9155
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I62317149cb8428f7d29e945a538f6cc8dd45342f
Gerrit-Change-Number: 9155
Gerrit-PatchSet: 3
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
[Impala-ASF-CR] IMPALA-6450: fix EventSequence::Start()
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker has posted comments on this change. ( http://gerrit.cloudera.org:8080/9155 )
Change subject: IMPALA-6450: fix EventSequence::Start()
......................................................................
Patch Set 4:
(1 comment)
> Patch Set 3: Code-Review+2
>
> Also not for this patch, but Lars, is there some test we are missing that would have caught the offset_ omission?
We could try adding a flag to spread out / delay the startup of local fragment instances, then start a query with multiple instances on a single worker, then assert that the event sequences of the delayed instances reflect the delay in increased timestamps. However, I'm not sure it's wort the effort. Alternatively, we could add a unit test as part of IMPALA-6469 (see my inline comment).
http://gerrit.cloudera.org:8080/#/c/9155/2/be/src/util/runtime-profile-counters.h
File be/src/util/runtime-profile-counters.h:
http://gerrit.cloudera.org:8080/#/c/9155/2/be/src/util/runtime-profile-counters.h@304
PS2, Line 304: offset_ = MonotonicStopWatch::Now() - start_time_ns;
: // TODO: IMPALA-4631: Occasionally we see MonotonicStopWatch::Now() return
: // (start_time_ns - 1), even though 'start_time_ns' was obtained using
: // MonotonicStopWatch::Now().
: DCHECK_GE(offset_, -1);
: sw_.Start();
> Not for this change, but I had to dig around to understand what this offset
I created IMPALA-6469 to track this. I don't fully understand how a) solves the issue though, since we don't have a sw_ of which we could take the start time.
--
To view, visit http://gerrit.cloudera.org:8080/9155
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I62317149cb8428f7d29e945a538f6cc8dd45342f
Gerrit-Change-Number: 9155
Gerrit-PatchSet: 4
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-Comment-Date: Thu, 01 Feb 2018 01:09:44 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6450: loosen DCHECK in EventSequence::Start()
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker has posted comments on this change. ( http://gerrit.cloudera.org:8080/9155 )
Change subject: IMPALA-6450: loosen DCHECK in EventSequence::Start()
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/9155/1/be/src/util/runtime-profile-counters.h
File be/src/util/runtime-profile-counters.h:
http://gerrit.cloudera.org:8080/#/c/9155/1/be/src/util/runtime-profile-counters.h@308
PS1, Line 308: DCHECK_GE(offset_, -1);
> That's a good point. The intent was to add it to sw_.ElapsedTime() in MarkE
Uh, I seem to have missed that. Tim, let me know if you'd like to fix it, or prefer for me to take over this change. I don't think clamping offset is necessary.
--
To view, visit http://gerrit.cloudera.org:8080/9155
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I62317149cb8428f7d29e945a538f6cc8dd45342f
Gerrit-Change-Number: 9155
Gerrit-PatchSet: 1
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-Comment-Date: Tue, 30 Jan 2018 01:15:21 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6450: fix EventSequence::Start()
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/9155 )
Change subject: IMPALA-6450: fix EventSequence::Start()
......................................................................
Patch Set 4: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/9155
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I62317149cb8428f7d29e945a538f6cc8dd45342f
Gerrit-Change-Number: 9155
Gerrit-PatchSet: 4
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-Comment-Date: Thu, 01 Feb 2018 01:43:11 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6450: fix EventSequence::Start()
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/9155 )
Change subject: IMPALA-6450: fix EventSequence::Start()
......................................................................
Patch Set 3:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1838/
--
To view, visit http://gerrit.cloudera.org:8080/9155
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I62317149cb8428f7d29e945a538f6cc8dd45342f
Gerrit-Change-Number: 9155
Gerrit-PatchSet: 3
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-Comment-Date: Wed, 31 Jan 2018 06:26:46 +0000
Gerrit-HasComments: No