Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/8215 )
Change subject: IMPALA-5142 EventSequence displays negative elapsed time. ...................................................................... Patch Set 2: (2 comments) http://gerrit.cloudera.org:8080/#/c/8215/2/be/src/util/runtime-profile.cc File be/src/util/runtime-profile.cc: http://gerrit.cloudera.org:8080/#/c/8215/2/be/src/util/runtime-profile.cc@658 PS2, Line 658: event_sequence.second->GetEvents(&events); : if (last == 0 && events.size() > 0) last = events.back().second; that looks like it could be inconsistent with the sorted order http://gerrit.cloudera.org:8080/#/c/8215/2/be/src/util/runtime-profile.cc@672 PS2, Line 672: runtime-profile-test.cc has a test case that expects GetEvents() to be in order. that case isn't correct if we are saying that GetEvents() returns events unsorted. Also, the comment for events_ says the events are in sorted order. I think we should either: (a) maintain events in sorted order, or (b) do this sort inside GetEvents() either way the property is that GetEvents() always returns events in increasing time order, which seems like the most straightforward API. -- To view, visit http://gerrit.cloudera.org:8080/8215 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8c944396d96473b17b453da3e913ffc56680a896 Gerrit-Change-Number: 8215 Gerrit-PatchSet: 2 Gerrit-Owner: Pranay Singh Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Pranay Singh Gerrit-Comment-Date: Wed, 25 Oct 2017 16:46:56 +0000 Gerrit-HasComments: Yes
