[Impala-ASF-CR] IMPALA-6450: fix EventSequence::Start()

["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