impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Pranay Singh (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-5142 EventSequence displays negative elapsed time.
Date Wed, 01 Nov 2017 23:01:41 GMT
Pranay Singh has posted comments on this change. ( http://gerrit.cloudera.org:8080/8215 )

Change subject: IMPALA-5142 EventSequence displays negative elapsed time.
......................................................................


Patch Set 5:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/8215/5/be/src/util/runtime-profile-counters.h
File be/src/util/runtime-profile-counters.h:

http://gerrit.cloudera.org:8080/#/c/8215/5/be/src/util/runtime-profile-counters.h@327
PS5, Line 327:        [](Event const &event1, Event const &event2) {
             :           return event1.second < event2.second;
> rather than creating a tmp eventlist_sorted, you should create a temp for t
I'm dropping this logic to call the sort() all the time to make the result more predictable
each time this function gets called and also since the event list will be small so sorting
won't be that expensive.


http://gerrit.cloudera.org:8080/#/c/8215/5/be/src/util/runtime-profile-counters.h@330
PS5, Line 330:  == false
> our style uses ! rather than == false
Since I'm not checking the event list to be sorted so I won't need this anymore.


http://gerrit.cloudera.org:8080/#/c/8215/4/be/src/util/runtime-profile-counters.h
File be/src/util/runtime-profile-counters.h:

http://gerrit.cloudera.org:8080/#/c/8215/4/be/src/util/runtime-profile-counters.h@326
PS4, Line 326:     bool eventlist_sorted = std::is_sorted(events_.begin(), events_.end(),
             :         [](Event const &event1, Event const &event2) {
             :           return event1.second < event2.second;
             :       });
             :     if (eventlist_sorted == false) {
             :       std::sort(events_.begin(), events_.end(),
             :           [](Event const &event1, Event const &event2) {
             :           return event1.second < event2.second;
             :       });
             :  
> the flipside is that this becomes a cliff in the sense that normally it wil
Done as suggested kept the functionality simple, removed the logic of having an extra check.



-- 
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: 5
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Pranay Singh
Gerrit-Comment-Date: Wed, 01 Nov 2017 23:01:41 +0000
Gerrit-HasComments: Yes

Mime
  • Unnamed multipart/alternative (inline, 8-Bit, 0 bytes)
View raw message