Bharath Vissapragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/13801 )
Change subject: IMPALA-5149: Provide query profile in JSON format ...................................................................... Patch Set 6: (17 comments) Some more minor comments, but generally lgtm. I'll let other reviewers take a look too. http://gerrit.cloudera.org:8080/#/c/13801/6/be/src/service/impala-http-handler.cc File be/src/service/impala-http-handler.cc: http://gerrit.cloudera.org:8080/#/c/13801/6/be/src/service/impala-http-handler.cc@270 PS6, Line 270: if (format != TRuntimeProfileFormat::JSON){ nit: Add a quick comment why? http://gerrit.cloudera.org:8080/#/c/13801/6/be/src/util/runtime-profile-counters.h File be/src/util/runtime-profile-counters.h: http://gerrit.cloudera.org:8080/#/c/13801/6/be/src/util/runtime-profile-counters.h@126 PS6, Line 126: { nit: space const { . http://gerrit.cloudera.org:8080/#/c/13801/6/be/src/util/runtime-profile-counters.h@128 PS6, Line 128: rapidjson::Value::MemberIterator type_itr = val->FindMember("kind"); Add DHCECK (type_itr != val->MemberEnd())? (multiple places below too) http://gerrit.cloudera.org:8080/#/c/13801/6/be/src/util/runtime-profile-counters.h@164 PS6, Line 164: { same (multiple places below) http://gerrit.cloudera.org:8080/#/c/13801/6/be/src/util/runtime-profile-counters.h@289 PS6, Line 289: val->RemoveMember("kind"); why this? http://gerrit.cloudera.org:8080/#/c/13801/6/be/src/util/runtime-profile-counters.h@403 PS6, Line 403: void ToJson(rapidjson::Document& document, rapidjson::Value* value) { move to cc file? http://gerrit.cloudera.org:8080/#/c/13801/6/be/src/util/runtime-profile-counters.h@421 PS6, Line 421: *value = event_sequence_rapid; instantiate value = ...directly in L407? http://gerrit.cloudera.org:8080/#/c/13801/6/be/src/util/runtime-profile-test.cc File be/src/util/runtime-profile-test.cc: http://gerrit.cloudera.org:8080/#/c/13801/6/be/src/util/runtime-profile-test.cc@1173 PS6, Line 1173: counter_a = profile_a->AddCounter("A", TUnit::UNIT); check other units too and assert the unit data? http://gerrit.cloudera.org:8080/#/c/13801/6/be/src/util/runtime-profile-test.cc@1187 PS6, Line 1187: EXPECT_TRUE(content.FindMember("info_strings")==content.MemberEnd()); some tests for info_strings and event_sequences too? http://gerrit.cloudera.org:8080/#/c/13801/6/be/src/util/runtime-profile-test.cc@1202 PS6, Line 1202: TEST(ToJson, TimeSeriesCounterToJsonTest){ check other TimeSeriesCounter implementations too? http://gerrit.cloudera.org:8080/#/c/13801/6/be/src/util/runtime-profile.cc File be/src/util/runtime-profile.cc: http://gerrit.cloudera.org:8080/#/c/13801/6/be/src/util/runtime-profile.cc@705 PS6, Line 705: rapidjson:: remove rapidjson classifiers with using... http://gerrit.cloudera.org:8080/#/c/13801/6/be/src/util/runtime-profile.cc@713 PS6, Line 713: allocator use d->GetAllocator() (avoid temp variable) http://gerrit.cloudera.org:8080/#/c/13801/6/be/src/util/runtime-profile.cc@735 PS6, Line 735: child_counter, counter_map,child_counter_map); formatting http://gerrit.cloudera.org:8080/#/c/13801/6/be/src/util/runtime-profile.cc@1590 PS6, Line 1590: document.GetAllocator()); formatting http://gerrit.cloudera.org:8080/#/c/13801/6/be/src/util/runtime-profile.cc@1600 PS6, Line 1600: document.GetAllocator()); formatting http://gerrit.cloudera.org:8080/#/c/13801/6/tests/hs2/test_hs2.py File tests/hs2/test_hs2.py: http://gerrit.cloudera.org:8080/#/c/13801/6/tests/hs2/test_hs2.py@659 PS6, Line 659: def test_get_profile_json(self): why not do this in the above test after L654? http://gerrit.cloudera.org:8080/#/c/13801/6/tests/webserver/test_web_pages.py File tests/webserver/test_web_pages.py: http://gerrit.cloudera.org:8080/#/c/13801/6/tests/webserver/test_web_pages.py@585 PS6, Line 585: def test_download_json_profile(self): merge with test_download_text_profile? You can do something like for format in ("text", "json"): ... -- 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: 6 Gerrit-Owner: Jiawei Wang <[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: Quanlong Huang <[email protected]> Gerrit-Reviewer: Sahil Takiar <[email protected]> Gerrit-Comment-Date: Sat, 13 Jul 2019 04:00:03 +0000 Gerrit-HasComments: Yes
