Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15236 )
Change subject: IMPALA-9381: on-demand conversion of runtime profile ...................................................................... Patch Set 3: (5 comments) http://gerrit.cloudera.org:8080/#/c/15236/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/15236/3//COMMIT_MSG@7 PS3, Line 7: lazy > I would replace "lazy" with "On demand" - to me lazy suggests that it is co Done http://gerrit.cloudera.org:8080/#/c/15236/3//COMMIT_MSG@21 PS3, Line 21: most compact representation of the profile and and it is > typo: and and Done http://gerrit.cloudera.org:8080/#/c/15236/3/be/src/service/impala-server.cc File be/src/service/impala-server.cc: http://gerrit.cloudera.org:8080/#/c/15236/3/be/src/service/impala-server.cc@826 PS3, Line 826: record > If you are already optimizing this path, then it would be nice to avoid thi Done. Also changed query_log_index_ to store direct references to the record since I think that's simpler. http://gerrit.cloudera.org:8080/#/c/15236/3/be/src/util/runtime-profile.cc File be/src/util/runtime-profile.cc: http://gerrit.cloudera.org:8080/#/c/15236/3/be/src/util/runtime-profile.cc@1114 PS3, Line 1114: scoped_ptr<Codec> decompressor; : MemTracker mem_tracker; : MemPool mem_pool(&mem_tracker); : const auto close_mem_tracker = MakeScopeExitTrigger([&mem_pool, &mem_tracker]() { : mem_pool.FreeAll(); : mem_tracker.Close(); : }); > nice to have: it could be useful during prototyping to move this logic to a I think the memory management isn't quite right for general use (could encourage bad practices). http://gerrit.cloudera.org:8080/#/c/15236/3/be/src/util/runtime-profile.cc@1122 PS3, Line 1122: DEFAULT > Using DEFAULT seems weird to me - I guess that the are tools that parse the DEFAULT actually means gzip (I think there might be a couple of gzip variants). All the consumers will expect this format. -- To view, visit http://gerrit.cloudera.org:8080/15236 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic2f5133cc146adc3b044cf4b64aae0a9688449fa Gerrit-Change-Number: 15236 Gerrit-PatchSet: 3 Gerrit-Owner: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Abhishek Rawat <ara...@cloudera.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Jiawei Wang <jiawei.wang.0...@gmail.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Mon, 24 Feb 2020 19:43:12 +0000 Gerrit-HasComments: Yes
