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 12: (9 comments) We don't really enforce any compatibility for our runtime profile in general now so counters may get added and deleted across releases. So, the json profile output may change as Impala code changes. In the future, when we have a better compatibility story for our profile, it may make sense to have a version string in the output. http://gerrit.cloudera.org:8080/#/c/13801/12/be/src/service/impala-server.cc File be/src/service/impala-server.cc: http://gerrit.cloudera.org:8080/#/c/13801/12/be/src/service/impala-server.cc@845 PS12, Line 845: if (!parse_ok){ The header comment says the output stream is not modified on error. May be worth double checking that json_output not modified on parse error. http://gerrit.cloudera.org:8080/#/c/13801/12/be/src/util/runtime-profile.h File be/src/util/runtime-profile.h: http://gerrit.cloudera.org:8080/#/c/13801/12/be/src/util/runtime-profile.h@618 PS12, Line 618: void ToJsonCounters(rapidjson::Value* parent, rapidjson::Document* d, : const string& counter_name, const CounterMap& counter_map, : const ChildCounterMap& child_counter_map) const; Please document each of the parameters. http://gerrit.cloudera.org:8080/#/c/13801/11/be/src/util/runtime-profile.h File be/src/util/runtime-profile.h: http://gerrit.cloudera.org:8080/#/c/13801/11/be/src/util/runtime-profile.h@126 PS11, Line 126: Get the actual Counter derived type Returns the name of the counter type. http://gerrit.cloudera.org:8080/#/c/13801/12/be/src/util/runtime-profile.cc File be/src/util/runtime-profile.cc: http://gerrit.cloudera.org:8080/#/c/13801/12/be/src/util/runtime-profile.cc@766 PS12, Line 766: _rapid I noticed that a lot of the Value variables have the "_rapid" suffix. Is "_json" a more appropriate suffix ? http://gerrit.cloudera.org:8080/#/c/13801/12/be/src/util/runtime-profile.cc@784 PS12, Line 784: info_strings_.find(key)->second. DCHECK(info_strings_.find(key) != info_strings_.end()); http://gerrit.cloudera.org:8080/#/c/13801/12/be/src/util/runtime-profile.cc@802 PS12, Line 802: nit: unnecessary blank line http://gerrit.cloudera.org:8080/#/c/13801/12/be/src/util/runtime-profile.cc@869 PS12, Line 869: nit: unnecessary blank line http://gerrit.cloudera.org:8080/#/c/13801/12/tests/webserver/test_web_pages.py File tests/webserver/test_web_pages.py: http://gerrit.cloudera.org:8080/#/c/13801/12/tests/webserver/test_web_pages.py@563 PS12, Line 563: ("Text", "Json") ["Text", "Json"] http://gerrit.cloudera.org:8080/#/c/13801/12/tests/webserver/test_web_pages.py@563 PS12, Line 563: download_format profile_format -- 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: 12 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: Fri, 02 Aug 2019 07:08:45 +0000 Gerrit-HasComments: Yes
