Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/13801 )
Change subject: IMPALA-5149: Provide query profile in JSON format ...................................................................... Patch Set 13: (14 comments) LGTM. Some more minor nits. The patch can be +2'ed after they are addressed. http://gerrit.cloudera.org:8080/#/c/13801/13/be/src/service/impala-server.cc File be/src/service/impala-server.cc: http://gerrit.cloudera.org:8080/#/c/13801/13/be/src/service/impala-server.cc@848 PS13, Line 848: $1 offset: $1 http://gerrit.cloudera.org:8080/#/c/13801/13/be/src/service/impala-server.cc@2026 PS13, Line 2026: json_profile_rapid( json_profile. No need for "_rapid". http://gerrit.cloudera.org:8080/#/c/13801/13/be/src/util/runtime-profile-test.cc File be/src/util/runtime-profile-test.cc: http://gerrit.cloudera.org:8080/#/c/13801/13/be/src/util/runtime-profile-test.cc@1166 PS13, Line 1166: RuntimeProfile* profile_a2 = RuntimeProfile::Create(&pool, "ProfileAb"); The name of the profile should probably match the variable name. http://gerrit.cloudera.org:8080/#/c/13801/13/be/src/util/runtime-profile-test.cc@1204 PS13, Line 1204: for (rapidjson::Value::ConstValueIterator itr = content["counters"].Begin(); : itr != content["counters"].End(); ++itr) { Use iterator style like the following. Same at other places. for (auto& itr : content) { http://gerrit.cloudera.org:8080/#/c/13801/13/be/src/util/runtime-profile-test.cc@1218 PS13, Line 1218: } Do we want to check there are no other counter types in content ? if (is normal counter) { elif (is high water mark counter) { } else { DCHECK(false) ? } or DCHECK(counter is either normal counter || counters is high watermark counter) http://gerrit.cloudera.org:8080/#/c/13801/13/be/src/util/runtime-profile.h File be/src/util/runtime-profile.h: http://gerrit.cloudera.org:8080/#/c/13801/13/be/src/util/runtime-profile.h@622 PS13, Line 622: get the counter detailed information based on its name A map of counter's name to counter http://gerrit.cloudera.org:8080/#/c/13801/13/be/src/util/runtime-profile.h@625 PS13, Line 625: child_counter_map This needs to be documented too. http://gerrit.cloudera.org:8080/#/c/13801/13/be/src/util/runtime-profile.cc File be/src/util/runtime-profile.cc: http://gerrit.cloudera.org:8080/#/c/13801/13/be/src/util/runtime-profile.cc@704 PS13, Line 704: the other information What does all the other information mean ? http://gerrit.cloudera.org:8080/#/c/13801/13/tests/webserver/test_web_pages.py File tests/webserver/test_web_pages.py: http://gerrit.cloudera.org:8080/#/c/13801/13/tests/webserver/test_web_pages.py@571 PS13, Line 571: download_link = "query_profile_plain_text?query_id={0}".format( : query_id) nit: one line http://gerrit.cloudera.org:8080/#/c/13801/13/tests/webserver/test_web_pages.py@587 PS13, Line 587: self.ROOT_URL + download_link, query, self.IMPALAD_TEST_PORT) nit: indent 4 for line continuation http://gerrit.cloudera.org:8080/#/c/13801/13/tests/webserver/test_web_pages.py@594 PS13, Line 594: Download Downloaded ? http://gerrit.cloudera.org:8080/#/c/13801/13/tests/webserver/test_web_pages.py@595 PS13, Line 595: Response text Json pofile http://gerrit.cloudera.org:8080/#/c/13801/13/tests/webserver/test_web_pages.py@597 PS13, Line 597: assert query_id in json_res["contents"]["profile_name"] assert query_id in json_res["contents"]["profile_name"], json_res to help debugging in case the assert fires. http://gerrit.cloudera.org:8080/#/c/13801/13/www/query_profile.tmpl File www/query_profile.tmpl: http://gerrit.cloudera.org:8080/#/c/13801/13/www/query_profile.tmpl@28 PS13, Line 28: (Choose Format) (available formats): -- To view, visit http://gerrit.cloudera.org:8080/13801 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8181ac818bf22207ca1deabd9220c397ae723ec1 Gerrit-Change-Number: 13801 Gerrit-PatchSet: 13 Gerrit-Owner: Jiawei Wang <[email protected]> Gerrit-Reviewer: Andrew Sherman <[email protected]> Gerrit-Reviewer: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: David Knupp <[email protected]> Gerrit-Reviewer: David Rorke <[email protected]> Gerrit-Reviewer: Greg Rahn <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Jiawei Wang <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Sahil Takiar <[email protected]> Gerrit-Comment-Date: Tue, 06 Aug 2019 01:32:27 +0000 Gerrit-HasComments: Yes
