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 <jiawei.w...@cloudera.com> Gerrit-Reviewer: Andrew Sherman <asher...@cloudera.com> Gerrit-Reviewer: Bharath Vissapragada <bhara...@cloudera.com> Gerrit-Reviewer: David Knupp <dkn...@cloudera.com> Gerrit-Reviewer: David Rorke <dro...@cloudera.com> Gerrit-Reviewer: Greg Rahn <gr...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Jiawei Wang <jiawei.w...@cloudera.com> Gerrit-Reviewer: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: Quanlong Huang <huangquanl...@gmail.com> Gerrit-Reviewer: Sahil Takiar <stak...@cloudera.com> Gerrit-Comment-Date: Fri, 02 Aug 2019 07:08:45 +0000 Gerrit-HasComments: Yes