Lars Volker has posted comments on this change. ( http://gerrit.cloudera.org:8080/9749 )
Change subject: IMPALA-6694: fix "buffer pool" child profile order ...................................................................... Patch Set 3: (4 comments) Logic looks good to me, had some minor questions. http://gerrit.cloudera.org:8080/#/c/9749/3/be/src/util/runtime-profile-test.cc File be/src/util/runtime-profile-test.cc: http://gerrit.cloudera.org:8080/#/c/9749/3/be/src/util/runtime-profile-test.cc@250 PS3, Line 250: child 2 child 1? http://gerrit.cloudera.org:8080/#/c/9749/3/be/src/util/runtime-profile.cc File be/src/util/runtime-profile.cc: http://gerrit.cloudera.org:8080/#/c/9749/3/be/src/util/runtime-profile.cc@207 PS3, Line 207: Update Could they also come from UpdateAverage()? http://gerrit.cloudera.org:8080/#/c/9749/3/be/src/util/runtime-profile.cc@345 PS3, Line 345: // if children are added after the first Update() call (IMPALA-6694). I think a very simple example would help to understand this, something like "first update sends B, D, second update sends A, B, C, D, this code makes sure that children_ is A, B, C, D afterwards." http://gerrit.cloudera.org:8080/#/c/9749/3/be/src/util/runtime-profile.cc@358 PS3, Line 358: insert_pos != children_.end() I can't think of a case where this happens without found_child == true. If we find the child in child_map_ it also has to be in the vector and we will find it. Is the intent here to handle corrupt profiles gracefully? If so, can we add an additional DCHECK? Or am I missing something? -- To view, visit http://gerrit.cloudera.org:8080/9749 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I230f0673edf20a846fdb13191b7a292d329c1bb8 Gerrit-Change-Number: 9749 Gerrit-PatchSet: 3 Gerrit-Owner: Tim Armstrong <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Wed, 28 Mar 2018 00:01:44 +0000 Gerrit-HasComments: Yes
