Andrew Sherman has posted comments on this change. ( http://gerrit.cloudera.org:8080/11977 )
Change subject: IMPALA-6924: Add child queries to profile in compute stats ...................................................................... Patch Set 4: Code-Review+1 (5 comments) This all looks good to me. I have comments that are more like questions so I can learn things. http://gerrit.cloudera.org:8080/#/c/11977/4/be/src/service/client-request-state.cc File be/src/service/client-request-state.cc: http://gerrit.cloudera.org:8080/#/c/11977/4/be/src/service/client-request-state.cc@572 PS4, Line 572: child_queries.push_back(ChildQuery(compute_stats_params.tbl_stats_query, this, I know you didn't write this line, but I see some people advocating that emplace_back is better than push_back as it can avoid a copy? http://gerrit.cloudera.org:8080/#/c/11977/4/be/src/service/impala-hs2-server.cc File be/src/service/impala-hs2-server.cc: http://gerrit.cloudera.org:8080/#/c/11977/4/be/src/service/impala-hs2-server.cc@907 PS4, Line 907: return_val.__set_profile(ss.str()); So this covers both the string and base64 cases? http://gerrit.cloudera.org:8080/#/c/11977/4/be/src/util/runtime-profile-test.cc File be/src/util/runtime-profile-test.cc: http://gerrit.cloudera.org:8080/#/c/11977/4/be/src/util/runtime-profile-test.cc@93 PS4, Line 93: EXPECT_TRUE(deserialized_profile->GetCounter("Not there") == NULL); == nullptr http://gerrit.cloudera.org:8080/#/c/11977/4/be/src/util/runtime-profile.cc File be/src/util/runtime-profile.cc: http://gerrit.cloudera.org:8080/#/c/11977/4/be/src/util/runtime-profile.cc@888 PS4, Line 888: uint32_t deserialized_len = result_len; Would there be any advantage to using a cast here? http://gerrit.cloudera.org:8080/#/c/11977/4/tests/query_test/test_observability.py File tests/query_test/test_observability.py: http://gerrit.cloudera.org:8080/#/c/11977/4/tests/query_test/test_observability.py@265 PS4, Line 265: matches = re.findall("Query \(id=.{,33}\)", results.runtime_profile) So 33 is the length of a query id? That made me think. -- To view, visit http://gerrit.cloudera.org:8080/11977 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5006c3b366d381eed4687e550cdfc463be3d1350 Gerrit-Change-Number: 11977 Gerrit-PatchSet: 4 Gerrit-Owner: Thomas Marshall <[email protected]> Gerrit-Reviewer: Andrew Sherman <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Thomas Marshall <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Tue, 04 Dec 2018 17:41:49 +0000 Gerrit-HasComments: Yes
