Tim Armstrong 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 2: (1 comment) This will be very useful. I had one high level question about the approach. Otherwise the code looked good. http://gerrit.cloudera.org:8080/#/c/11977/2/be/src/service/child-query.cc File be/src/service/child-query.cc: http://gerrit.cloudera.org:8080/#/c/11977/2/be/src/service/child-query.cc@104 PS2, Line 104: profile_->AddInfoString("Profile", get_profile_resp.profile); Did you think about splicing the profiles together with AddChild(). It feels a bit wrong putting the text version of the profile in the machine-readable profile since it mixes the two formats and prevents tools from working properly. Or is the idea that a profile analysis tool should be able to link up the queries itself if needed based on the query ids? And this is just for human convenience? If we're going down that path maybe it would make sense to add machine-readable metadata to the thrift struct with the child query ids - it would be good to have a clear story about how a tool should use this. -- 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: 2 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: Wed, 28 Nov 2018 00:34:31 +0000 Gerrit-HasComments: Yes
