[Impala-ASF-CR] IMPALA-5149: Provide query profile in JSON format

["Michael Ho (Code Review)" <ge...@cloudera.org>]

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 <ji...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: David Rorke <dr...@cloudera.com>
Gerrit-Reviewer: Greg Rahn <gr...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jiawei Wang <ji...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Comment-Date: Fri, 02 Aug 2019 07:08:45 +0000
Gerrit-HasComments: Yes