You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Jiawei Wang (Code Review)" <ge...@cloudera.org> on 2019/07/03 21:29:15 UTC

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

Jiawei Wang has uploaded this change for review. ( http://gerrit.cloudera.org:8080/13801


Change subject: IMPALA-5149: Provide query profile in JSON format
......................................................................

IMPALA-5149: Provide query profile in JSON format

Today there is a text and Thrift version of the query profile, but
it would be useful to have a JSON version for portability and machine
consumption. The ultimate goal is to have a Download JSON format
profile link along with the other two formats.

Design Document:
https://docs.google.com/document/d/
15P_Lmjf1rlZUD4PZLXdUwTE8Lv_ME3NQ9Yf4MITzO2M/edit

Change-Id: I8181ac818bf22207ca1deabd9220c397ae723ec1
---
M be/src/service/impala-beeswax-server.cc
M be/src/service/impala-hs2-server.cc
M be/src/service/impala-http-handler.cc
M be/src/service/impala-http-handler.h
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/util/runtime-profile-counters.h
M be/src/util/runtime-profile.cc
M be/src/util/runtime-profile.h
M common/thrift/RuntimeProfile.thrift
M www/query_profile.tmpl
11 files changed, 363 insertions(+), 11 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/01/13801/1
-- 
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: newchange
Gerrit-Change-Id: I8181ac818bf22207ca1deabd9220c397ae723ec1
Gerrit-Change-Number: 13801
Gerrit-PatchSet: 1
Gerrit-Owner: Jiawei Wang <ji...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: David Rorke <dr...@cloudera.com>
Gerrit-Reviewer: Greg Rahn <gr...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>

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

Posted by "Jiawei Wang (Code Review)" <ge...@cloudera.org>.
Jiawei Wang has posted comments on this change. ( http://gerrit.cloudera.org:8080/13801 )

Change subject: IMPALA-5149: Provide query profile in JSON format
......................................................................


Patch Set 10:

(23 comments)

Thanks for the valuable feedback! Looking forward to the next step and the version control discussion.

http://gerrit.cloudera.org:8080/#/c/13801/9/be/src/service/impala-hs2-server.cc
File be/src/service/impala-hs2-server.cc:

http://gerrit.cloudera.org:8080/#/c/13801/9/be/src/service/impala-hs2-server.cc@988
PS9, Line 988:   } else {
> nit: missing blank space after }
Done


http://gerrit.cloudera.org:8080/#/c/13801/9/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:

http://gerrit.cloudera.org:8080/#/c/13801/9/be/src/service/impala-server.cc@101
PS9, Line 101: #include "common/names.h"
             : 
             : using boost::adopt_lock_t;
             : using boost::algorithm::is_an
> Why not group these together with other rapidjson #include above ? There se
Done


http://gerrit.cloudera.org:8080/#/c/13801/9/be/src/service/impala-server.cc@783
PS9, Line 783: turn Status::OK();
> DCHECK_EQ(format, TRuntimeProfileFormat::STRING);
Done


http://gerrit.cloudera.org:8080/#/c/13801/9/be/src/service/impala-server.cc@818
PS9, Line 818: 
> DCHECK_EQ(format, TRuntimeProfileFormat::STRING);
Done


http://gerrit.cloudera.org:8080/#/c/13801/9/be/src/util/runtime-profile-counters.h
File be/src/util/runtime-profile-counters.h:

http://gerrit.cloudera.org:8080/#/c/13801/9/be/src/util/runtime-profile-counters.h@126
PS9, Line 126:   string CounterType() const override {
> void ToJson(rapidjson::Document& document, rapidjson::Value* val) const ove
Done


http://gerrit.cloudera.org:8080/#/c/13801/9/be/src/util/runtime-profile-counters.h@131
PS9, Line 131: 
> Actually, do you need to output this in json ? It seems sufficient to just 
Done


http://gerrit.cloudera.org:8080/#/c/13801/9/be/src/util/runtime-profile-counters.h@165
PS9, Line 165:  private:
             :   SampleFunction counter_fn_;
             : };
             : 
             : /// An AveragedCounter maintains a set of counters and its value is the
             : /// average of the values in that set. The average is updated through calls
             : ///
> If you take the suggestion in Counter::ToJson(), you probably don't need th
Done


http://gerrit.cloudera.org:8080/#/c/13801/9/be/src/util/runtime-profile-counters.h@219
PS9, Line 219:  private:
             :   /// Map from counters to their existing values. Modified via UpdateCounter().
             :   boost::unordered_map<Counter*, int64_t> counter
> See comments in Counter::ToJson() on why we may not need this.
Done


http://gerrit.cloudera.org:8080/#/c/13801/9/be/src/util/runtime-profile-counters.h@222
PS9, Line 222: 
             :   /// Current sums of values from counter_value_map_. Only one of these is used,
             :   /// depending on the unit of the counter. current_double_sum_ is used for
             :   /// DOUBLE_VALUE, current_int_sum_ otherwise.
             :   dou
> Same question as above. Do you need to store the internal states (i.e. sum)
Done


http://gerrit.cloudera.org:8080/#/c/13801/9/be/src/util/runtime-profile-counters.h@290
PS9, Line 290:   /// Summary statistics of values seen so far.
> virtual override
Done


http://gerrit.cloudera.org:8080/#/c/13801/9/be/src/util/runtime-profile-counters.h@292
PS9, Line 292: t64_t max_;
> Why is "kind" removed ? May be worth adding a comment here.
Done


http://gerrit.cloudera.org:8080/#/c/13801/9/be/src/util/runtime-profile-counters.h@406
PS9, Line 406: void SortEvents() {
> Please add a comment about the output format.
Done


http://gerrit.cloudera.org:8080/#/c/13801/9/be/src/util/runtime-profile-counters.h@452
PS9, Line 452: ///	“unit” : xxx,
> It'd be nice to add a comment about the output format.
Done


http://gerrit.cloudera.org:8080/#/c/13801/9/be/src/util/runtime-profile-counters.h@583
PS9, Line 583: 
             :   void Add(int64_t delta) override {
             :     DCHECK(false);
             :   }
             : 
             :   string CounterType() const override {
             :    
> Similar to comments above, this may not be needed since Counter::ToJson() m
Done


http://gerrit.cloudera.org:8080/#/c/13801/9/be/src/util/runtime-profile.h
File be/src/util/runtime-profile.h:

http://gerrit.cloudera.org:8080/#/c/13801/9/be/src/util/runtime-profile.h@123
PS9, Line 123: counter_name, value, kind, unit
> is this list actually accurate ? What about "kind"  and "unit" ?
Done


http://gerrit.cloudera.org:8080/#/c/13801/9/be/src/util/runtime-profile.cc
File be/src/util/runtime-profile.cc:

http://gerrit.cloudera.org:8080/#/c/13801/9/be/src/util/runtime-profile.cc@709
PS9, Line 709: }
> nit: unnecessary blank line
Done


http://gerrit.cloudera.org:8080/#/c/13801/9/be/src/util/runtime-profile.cc@797
PS9, Line 797:       Value event_sequences_rapid(kArrayType);
> nit: formatting
Done


http://gerrit.cloudera.org:8080/#/c/13801/9/be/src/util/runtime-profile.cc@821
PS9, Line 821:       Value summary_stats_counters_rapid(kArrayType);
> formatting
Done


http://gerrit.cloudera.org:8080/#/c/13801/9/be/src/util/runtime-profile.cc@855
PS9, Line 855: Create copy of
> incorrect comment from copy-n-paste
Done


http://gerrit.cloudera.org:8080/#/c/13801/9/be/src/util/runtime-profile.cc@1582
PS9, Line 1582: 
> Use value() instead in case derived class overrides value().
Done


http://gerrit.cloudera.org:8080/#/c/13801/9/be/src/util/runtime-profile.cc@1583
PS9, Line 1583: Member("value", value(), document.
> Does it make sense to DCHECK that this cannot be _TUnit_VALUES_TO_NAMES.end
Done


http://gerrit.cloudera.org:8080/#/c/13801/9/be/src/util/runtime-profile.cc@1585
PS9, Line 1585: _TO_NAMES
> Instead of just adding "Counter" here, how about we implement a counter typ
Done


http://gerrit.cloudera.org:8080/#/c/13801/9/be/src/util/runtime-profile.cc@1594
PS9, Line 1594: ck> lock(lock_);
> Same question about DCHECK as above.
Done



-- 
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: 10
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, 26 Jul 2019 20:55:00 +0000
Gerrit-HasComments: Yes

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

Posted by "Jiawei Wang (Code Review)" <ge...@cloudera.org>.
Jiawei Wang has uploaded a new patch set (#2). ( http://gerrit.cloudera.org:8080/13801 )

Change subject: IMPALA-5149: Provide query profile in JSON format
......................................................................

IMPALA-5149: Provide query profile in JSON format

Today there is a text and Thrift version of the query profile, but
it would be useful to have a JSON version for portability and machine
consumption. The ultimate goal is to have a Download JSON format
profile link along with the other two formats.

Design Document:
https://docs.google.com/document/d/
15P_Lmjf1rlZUD4PZLXdUwTE8Lv_ME3NQ9Yf4MITzO2M/edit

Change-Id: I8181ac818bf22207ca1deabd9220c397ae723ec1
---
M be/src/service/impala-beeswax-server.cc
M be/src/service/impala-hs2-server.cc
M be/src/service/impala-http-handler.cc
M be/src/service/impala-http-handler.h
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/util/runtime-profile-counters.h
M be/src/util/runtime-profile.cc
M be/src/util/runtime-profile.h
M common/thrift/RuntimeProfile.thrift
M www/query_profile.tmpl
11 files changed, 364 insertions(+), 11 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/01/13801/2
-- 
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: newpatchset
Gerrit-Change-Id: I8181ac818bf22207ca1deabd9220c397ae723ec1
Gerrit-Change-Number: 13801
Gerrit-PatchSet: 2
Gerrit-Owner: Jiawei Wang <ji...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: David Rorke <dr...@cloudera.com>
Gerrit-Reviewer: Greg Rahn <gr...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>

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

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/4146/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


-- 
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 <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: Tue, 06 Aug 2019 00:26:11 +0000
Gerrit-HasComments: No

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

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13801 )

Change subject: IMPALA-5149: Provide query profile in JSON format
......................................................................


Patch Set 7:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/13801/7/be/src/util/runtime-profile-test.cc
File be/src/util/runtime-profile-test.cc:

http://gerrit.cloudera.org:8080/#/c/13801/7/be/src/util/runtime-profile-test.cc@1302
PS7, Line 1302:   
line has trailing whitespace



-- 
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: 7
Gerrit-Owner: Jiawei Wang <ji...@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: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Comment-Date: Mon, 15 Jul 2019 23:51:46 +0000
Gerrit-HasComments: Yes

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

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13801 )

Change subject: IMPALA-5149: Provide query profile in JSON format
......................................................................


Patch Set 3:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/13801/3/tests/webserver/test_web_pages.py
File tests/webserver/test_web_pages.py:

http://gerrit.cloudera.org:8080/#/c/13801/3/tests/webserver/test_web_pages.py@613
PS3, Line 613: j
flake8: F841 local variable 'json_object' is assigned to but never used


http://gerrit.cloudera.org:8080/#/c/13801/3/tests/webserver/test_web_pages.py@614
PS3, Line 614: e
flake8: F841 local variable 'e' is assigned to but never used


http://gerrit.cloudera.org:8080/#/c/13801/3/tests/webserver/test_web_pages.py@616
PS3, Line 616: 
flake8: W292 no newline at end of file



-- 
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: 3
Gerrit-Owner: Jiawei Wang <ji...@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: Sahil Takiar <st...@cloudera.com>
Gerrit-Comment-Date: Wed, 10 Jul 2019 01:32:39 +0000
Gerrit-HasComments: Yes

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

Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
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 2:

(19 comments)

Great work. Initial round of comments. Will take a deeper look in the next round. Lets also add some e-e tests to start with (look at test_web_pages.py). Feel free to add any unit tests in runtime-profile-test.cc.

http://gerrit.cloudera.org:8080/#/c/13801/2//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/13801/2//COMMIT_MSG@14
PS2, Line 14: Design Document:
            : https://docs.google.com/document/d/
            : 15P_Lmjf1rlZUD4PZLXdUwTE8Lv_ME3NQ9Yf4MITzO2M/edit
Lets get rid of this, might get stale, I'd suggest to add a PDF version to the jira.


http://gerrit.cloudera.org:8080/#/c/13801/2/be/src/service/impala-http-handler.h
File be/src/service/impala-http-handler.h:

http://gerrit.cloudera.org:8080/#/c/13801/2/be/src/service/impala-http-handler.h@126
PS2, Line 126: json
nit: lets stick to JSON (caps) for consistency everywhere.


http://gerrit.cloudera.org:8080/#/c/13801/2/be/src/service/impala-http-handler.cc
File be/src/service/impala-http-handler.cc:

http://gerrit.cloudera.org:8080/#/c/13801/2/be/src/service/impala-http-handler.cc@266
PS2, Line 266: json_output_rapid
curious why not use 'document' directly?


http://gerrit.cloudera.org:8080/#/c/13801/2/be/src/service/impala-http-handler.cc@272
PS2, Line 272: Value output_rapid(json_output_rapid["contents"], document->GetAllocator());
             :     document->AddMember("contents", output_rapid, document->GetAllocator());
             :   } else{
             :     document->AddMember("contents", rapidjson::StringRef(ss.str().c_str()),
             :         document->GetAllocator());
see my comment below, lets not do this here.


http://gerrit.cloudera.org:8080/#/c/13801/2/be/src/service/impala-http-handler.cc@283
PS2, Line 283:   document->AddMember(rapidjson::StringRef(Webserver::ENABLE_RAW_HTML_KEY), true,
why this?


http://gerrit.cloudera.org:8080/#/c/13801/2/be/src/service/impala-http-handler.cc@290
PS2, Line 290:   document->AddMember(rapidjson::StringRef(Webserver::ENABLE_RAW_HTML_KEY), true,
lets move this back to QueryProfileHelper


http://gerrit.cloudera.org:8080/#/c/13801/2/be/src/service/impala-http-handler.cc@300
PS2, Line 300:   document->RemoveMember("__common__");
Let's not do this to be consistent with other endpoints. I think we need to fix it properly in the webserver code for all of the text based content types.


http://gerrit.cloudera.org:8080/#/c/13801/2/be/src/service/impala-server.h
File be/src/service/impala-server.h:

http://gerrit.cloudera.org:8080/#/c/13801/2/be/src/service/impala-server.h@51
PS2, Line 51: 
nit: remove


http://gerrit.cloudera.org:8080/#/c/13801/2/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:

http://gerrit.cloudera.org:8080/#/c/13801/2/be/src/service/impala-server.cc@759
PS2, Line 759: rapidjson::
We pull the rapidjson ns above. We can get rid of this qualifier.


http://gerrit.cloudera.org:8080/#/c/13801/2/be/src/service/impala-server.cc@807
PS2, Line 807: } else if (format == TRuntimeProfileFormat::JSON) {
Check formatting. Like in other cases, we need to handle the parse errors here.


http://gerrit.cloudera.org:8080/#/c/13801/2/be/src/service/impala-server.cc@1981
PS2, Line 1981: rapidjson::
remove


http://gerrit.cloudera.org:8080/#/c/13801/2/be/src/util/runtime-profile.h
File be/src/util/runtime-profile.h:

http://gerrit.cloudera.org:8080/#/c/13801/2/be/src/util/runtime-profile.h@25
PS2, Line 25: #include <rapidjson/document.h>
forward declare?


http://gerrit.cloudera.org:8080/#/c/13801/2/be/src/util/runtime-profile.h@125
PS2, Line 125: rapidjson::Document* document
general notation is to use output params as ptrs and other input params as const references. 

https://google.github.io/styleguide/cppguide.html#Output_Parameters


http://gerrit.cloudera.org:8080/#/c/13801/2/be/src/util/runtime-profile.h@125
PS2, Line 125:  {
virtual void ToJson(...) const {


http://gerrit.cloudera.org:8080/#/c/13801/2/be/src/util/runtime-profile.h@131
PS2, Line 131:     }
Lets move the function definition to the cpp file


http://gerrit.cloudera.org:8080/#/c/13801/2/be/src/util/runtime-profile.cc
File be/src/util/runtime-profile.cc:

http://gerrit.cloudera.org:8080/#/c/13801/2/be/src/util/runtime-profile.cc@707
PS2, Line 707:   rapidjson::Document::AllocatorType& allocator = d->GetAllocator();
nit: auto


http://gerrit.cloudera.org:8080/#/c/13801/2/be/src/util/runtime-profile.cc@717
PS2, Line 717:                                          rapidjson::Document* d,
we use 4 space indents

https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=65868536


http://gerrit.cloudera.org:8080/#/c/13801/2/be/src/util/runtime-profile.cc@846
PS2, Line 846:     lock_guard<SpinLock> l(counter_map_lock_);
lot of code duplication between this and PrettyPrint(). Factor out into helpers?


http://gerrit.cloudera.org:8080/#/c/13801/2/www/query_profile.tmpl
File www/query_profile.tmpl:

http://gerrit.cloudera.org:8080/#/c/13801/2/www/query_profile.tmpl@35
PS2, Line 35:   <a href="/query_profile_json?query_id={{query_id}}"
nit: As discussed, I think we could make the UI a bit clean by adding a small popup selector to pick the format to download. Right now, all 3 options are stacked one below other.



-- 
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: 2
Gerrit-Owner: Jiawei Wang <ji...@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-Comment-Date: Tue, 09 Jul 2019 06:21:45 +0000
Gerrit-HasComments: Yes

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

Posted by "Jiawei Wang (Code Review)" <ge...@cloudera.org>.
Jiawei Wang has uploaded a new patch set (#5). ( http://gerrit.cloudera.org:8080/13801 )

Change subject: IMPALA-5149: Provide query profile in JSON format
......................................................................

IMPALA-5149: Provide query profile in JSON format

Description:
Today there is a text and Thrift version of the query profile, but
it would be useful to have a JSON version for portability and machine
consumption. The ultimate goal is to have a Download JSON format
profile link along with the other two formats.

Modification:
1.Add Json format download option in impala server
2.Add ToJson() function for RuntimeProfile, Counters, EventSequence
3.Add JSON format into QueryStateRecord
3.Add tests for E2E test
3.Modify query profile page to different download option

Tests:
E2E tests:
* tests/webserver/test_web_pages.py - test_download_json_profile

Change-Id: I8181ac818bf22207ca1deabd9220c397ae723ec1
---
M be/src/service/impala-beeswax-server.cc
M be/src/service/impala-hs2-server.cc
M be/src/service/impala-http-handler.cc
M be/src/service/impala-http-handler.h
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/util/runtime-profile-counters.h
M be/src/util/runtime-profile.cc
M be/src/util/runtime-profile.h
M common/thrift/RuntimeProfile.thrift
M tests/webserver/test_web_pages.py
M www/query_profile.tmpl
12 files changed, 405 insertions(+), 26 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/01/13801/5
-- 
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: newpatchset
Gerrit-Change-Id: I8181ac818bf22207ca1deabd9220c397ae723ec1
Gerrit-Change-Number: 13801
Gerrit-PatchSet: 5
Gerrit-Owner: Jiawei Wang <ji...@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: Sahil Takiar <st...@cloudera.com>

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

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/3859/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


-- 
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 <ji...@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: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Comment-Date: Thu, 11 Jul 2019 00:39:21 +0000
Gerrit-HasComments: No

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

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13801 )

Change subject: IMPALA-5149: Provide query profile in JSON format
......................................................................


Patch Set 11:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/4035/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


-- 
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: 11
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, 26 Jul 2019 21:38:17 +0000
Gerrit-HasComments: No

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

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13801 )

Change subject: IMPALA-5149: Provide query profile in JSON format
......................................................................


Patch Set 15: Code-Review+2


-- 
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: 15
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: Wed, 07 Aug 2019 17:01:48 +0000
Gerrit-HasComments: No

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

Posted by "Jiawei Wang (Code Review)" <ge...@cloudera.org>.
Jiawei Wang has uploaded a new patch set (#12). ( http://gerrit.cloudera.org:8080/13801 )

Change subject: IMPALA-5149: Provide query profile in JSON format
......................................................................

IMPALA-5149: Provide query profile in JSON format

Description:
Today there is a text and Thrift version of the query profile, but
it would be useful to have a JSON version for portability and machine
consumption. The ultimate goal is to have a Download JSON format
profile link along with the other two formats.

Modification:
1.Add Json format download option in impalad profile page
2.Add ToJson() function for RuntimeProfile, Counters, EventSequence
3.Add JSON format into QueryStateRecord
4.Add tests for E2E test, unit test, hs2 test
5.Modify query profile page to different download option
6.Modify HS2 server to support get JSON format profile

Future compatibility:
The schema of the JSON format can be changed in the future with
the standardization of Profile and Counter structure.

Tests:
E2E tests:
tests/webserver/test_web_pages.py - test_download_profile
 * merge text and json format download together
HS2 tests:
tests/hs2/test_hs2.py - test_get_profile
 * add json format test
BE Unit tests:
be/src/util/runtime-profile-test.cc
 * ToJson.RuntimeProfileToJsonTest
 * ToJson.EmptyTest
 * ToJson.EventSequenceToJsonTest
 * ToJson.TimeSeriesCounterToJsonTest

Change-Id: I8181ac818bf22207ca1deabd9220c397ae723ec1
---
M be/src/service/impala-beeswax-server.cc
M be/src/service/impala-hs2-server.cc
M be/src/service/impala-http-handler.cc
M be/src/service/impala-http-handler.h
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/util/CMakeLists.txt
M be/src/util/runtime-profile-counters.h
M be/src/util/runtime-profile-test.cc
M be/src/util/runtime-profile.cc
M be/src/util/runtime-profile.h
M common/thrift/ImpalaService.thrift
M common/thrift/RuntimeProfile.thrift
M tests/hs2/test_hs2.py
M tests/webserver/test_web_pages.py
M www/query_profile.tmpl
16 files changed, 642 insertions(+), 60 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/01/13801/12
-- 
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: newpatchset
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>

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

Posted by "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 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 <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: Tue, 06 Aug 2019 01:32:27 +0000
Gerrit-HasComments: Yes

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

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13801 )

Change subject: IMPALA-5149: Provide query profile in JSON format
......................................................................


Patch Set 10:

(10 comments)

http://gerrit.cloudera.org:8080/#/c/13801/10/be/src/util/runtime-profile-counters.h
File be/src/util/runtime-profile-counters.h:

http://gerrit.cloudera.org:8080/#/c/13801/10/be/src/util/runtime-profile-counters.h@396
PS10, Line 396:   ///	“offset” : xxx,
tab used for whitespace


http://gerrit.cloudera.org:8080/#/c/13801/10/be/src/util/runtime-profile-counters.h@397
PS10, Line 397:   ///	“events”: [{
tab used for whitespace


http://gerrit.cloudera.org:8080/#/c/13801/10/be/src/util/runtime-profile-counters.h@398
PS10, Line 398:   ///	    “label”: xxx,
tab used for whitespace


http://gerrit.cloudera.org:8080/#/c/13801/10/be/src/util/runtime-profile-counters.h@399
PS10, Line 399:   ///	    “timestamp”: xxx
tab used for whitespace


http://gerrit.cloudera.org:8080/#/c/13801/10/be/src/util/runtime-profile-counters.h@400
PS10, Line 400:   ///	},{...}]
tab used for whitespace


http://gerrit.cloudera.org:8080/#/c/13801/10/be/src/util/runtime-profile-counters.h@451
PS10, Line 451:   ///	“counter_name” : xxx,
tab used for whitespace


http://gerrit.cloudera.org:8080/#/c/13801/10/be/src/util/runtime-profile-counters.h@452
PS10, Line 452:   ///	“unit” : xxx,
tab used for whitespace


http://gerrit.cloudera.org:8080/#/c/13801/10/be/src/util/runtime-profile-counters.h@453
PS10, Line 453:   ///	“num” : xxx,
tab used for whitespace


http://gerrit.cloudera.org:8080/#/c/13801/10/be/src/util/runtime-profile-counters.h@454
PS10, Line 454:   ///	“period” : xxx,
tab used for whitespace


http://gerrit.cloudera.org:8080/#/c/13801/10/be/src/util/runtime-profile-counters.h@455
PS10, Line 455:   ///	“data”: “x,x,x,x”
tab used for whitespace



-- 
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: 10
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, 26 Jul 2019 20:54:45 +0000
Gerrit-HasComments: Yes

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

Posted by "Jiawei Wang (Code Review)" <ge...@cloudera.org>.
Jiawei Wang has uploaded a new patch set (#4). ( http://gerrit.cloudera.org:8080/13801 )

Change subject: IMPALA-5149: Provide query profile in JSON format
......................................................................

IMPALA-5149: Provide query profile in JSON format

Description:
Today there is a text and Thrift version of the query profile, but
it would be useful to have a JSON version for portability and machine
consumption. The ultimate goal is to have a Download JSON format
profile link along with the other two formats.

Modification:
1.Add Json format download option in impala server
2.Add ToJson() function for RuntimeProfile, Counters, EventSequence
3.Add JSON format into QueryStateRecord
3.Add tests for E2E test
3.Modify query profile page to different download option

Tests:
E2E tests:
* tests/webserver/test_web_pages.py - test_download_json_profile

Change-Id: I8181ac818bf22207ca1deabd9220c397ae723ec1
---
M be/src/service/impala-beeswax-server.cc
M be/src/service/impala-hs2-server.cc
M be/src/service/impala-http-handler.cc
M be/src/service/impala-http-handler.h
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/util/runtime-profile-counters.h
M be/src/util/runtime-profile.cc
M be/src/util/runtime-profile.h
M common/thrift/RuntimeProfile.thrift
M tests/webserver/test_web_pages.py
M www/query_profile.tmpl
12 files changed, 410 insertions(+), 26 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/01/13801/4
-- 
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: newpatchset
Gerrit-Change-Id: I8181ac818bf22207ca1deabd9220c397ae723ec1
Gerrit-Change-Number: 13801
Gerrit-PatchSet: 4
Gerrit-Owner: Jiawei Wang <ji...@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: Sahil Takiar <st...@cloudera.com>

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

Posted by "Jiawei Wang (Code Review)" <ge...@cloudera.org>.
Hello Bharath Vissapragada, Greg Rahn, Quanlong Huang, David Rorke, David Knupp, Sahil Takiar, Impala Public Jenkins, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/13801

to look at the new patch set (#8).

Change subject: IMPALA-5149: Provide query profile in JSON format
......................................................................

IMPALA-5149: Provide query profile in JSON format

Description:
Today there is a text and Thrift version of the query profile, but
it would be useful to have a JSON version for portability and machine
consumption. The ultimate goal is to have a Download JSON format
profile link along with the other two formats.

Modification:
1.Add Json format download option in impalad profile page
2.Add ToJson() function for RuntimeProfile, Counters, EventSequence
3.Add JSON format into QueryStateRecord
4.Add tests for E2E test, unit test, hs2 test
5.Modify query profile page to different download option
6.Modify HS2 server to support get JSON format profile

Tests:
E2E tests:
tests/webserver/test_web_pages.py - test_download_profile
 * merge text and json format download together
HS2 tests:
tests/hs2/test_hs2.py - test_get_profile
 * add json format test
BE Unit tests:
be/src/util/runtime-profile-test.cc
 * ToJson.RuntimeProfileToJsonTest
 * ToJson.EmptyTest
 * ToJson.EventSequenceToJsonTest
 * ToJson.TimeSeriesCounterToJsonTest

Change-Id: I8181ac818bf22207ca1deabd9220c397ae723ec1
---
M be/src/service/impala-beeswax-server.cc
M be/src/service/impala-hs2-server.cc
M be/src/service/impala-http-handler.cc
M be/src/service/impala-http-handler.h
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/util/CMakeLists.txt
M be/src/util/runtime-profile-counters.h
M be/src/util/runtime-profile-test.cc
M be/src/util/runtime-profile.cc
M be/src/util/runtime-profile.h
M common/thrift/ImpalaService.thrift
M common/thrift/RuntimeProfile.thrift
M tests/hs2/test_hs2.py
M tests/webserver/test_web_pages.py
M www/query_profile.tmpl
16 files changed, 611 insertions(+), 45 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/01/13801/8
-- 
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: newpatchset
Gerrit-Change-Id: I8181ac818bf22207ca1deabd9220c397ae723ec1
Gerrit-Change-Number: 13801
Gerrit-PatchSet: 8
Gerrit-Owner: Jiawei Wang <ji...@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: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>

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

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13801 )

Change subject: IMPALA-5149: Provide query profile in JSON format
......................................................................


Patch Set 1:

Build Failed 

https://jenkins.impala.io/job/gerrit-code-review-checks/3821/ : Initial code review checks failed. See linked job for details on the failure.


-- 
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: 1
Gerrit-Owner: Jiawei Wang <ji...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@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-Comment-Date: Wed, 03 Jul 2019 22:09:39 +0000
Gerrit-HasComments: No

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

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13801 )

Change subject: IMPALA-5149: Provide query profile in JSON format
......................................................................


Patch Set 3:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/3852/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


-- 
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: 3
Gerrit-Owner: Jiawei Wang <ji...@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: Sahil Takiar <st...@cloudera.com>
Gerrit-Comment-Date: Wed, 10 Jul 2019 02:13:43 +0000
Gerrit-HasComments: No

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

Posted by "Sahil Takiar (Code Review)" <ge...@cloudera.org>.
Sahil Takiar has posted comments on this change. ( http://gerrit.cloudera.org:8080/13801 )

Change subject: IMPALA-5149: Provide query profile in JSON format
......................................................................


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/13801/2/be/src/service/impala-hs2-server.cc
File be/src/service/impala-hs2-server.cc:

http://gerrit.cloudera.org:8080/#/c/13801/2/be/src/service/impala-hs2-server.cc@979
PS2, Line 979:   if (request.format == TRuntimeProfileFormat::THRIFT) {
             :     return_val.__set_thrift_profile(thrift_profile);
             :   } else {
             :     DCHECK(request.format == TRuntimeProfileFormat::STRING
             :         || request.format == TRuntimeProfileFormat::BASE64);
             :     return_val.__set_profile(ss.str());
             :   }
should there be an equivalent here for 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: 2
Gerrit-Owner: Jiawei Wang <ji...@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: Sahil Takiar <st...@cloudera.com>
Gerrit-Comment-Date: Wed, 10 Jul 2019 03:58:33 +0000
Gerrit-HasComments: Yes

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

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13801 )

Change subject: IMPALA-5149: Provide query profile in JSON format
......................................................................


Patch Set 4:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/13801/4/tests/webserver/test_web_pages.py
File tests/webserver/test_web_pages.py:

http://gerrit.cloudera.org:8080/#/c/13801/4/tests/webserver/test_web_pages.py@613
PS4, Line 613: j
flake8: F841 local variable 'json_object' is assigned to but never used


http://gerrit.cloudera.org:8080/#/c/13801/4/tests/webserver/test_web_pages.py@614
PS4, Line 614: e
flake8: F841 local variable 'e' is assigned to but never used


http://gerrit.cloudera.org:8080/#/c/13801/4/tests/webserver/test_web_pages.py@616
PS4, Line 616: 
flake8: W292 no newline at end of file



-- 
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: 4
Gerrit-Owner: Jiawei Wang <ji...@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: Sahil Takiar <st...@cloudera.com>
Gerrit-Comment-Date: Wed, 10 Jul 2019 01:36:59 +0000
Gerrit-HasComments: Yes

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

Posted by "Jiawei Wang (Code Review)" <ge...@cloudera.org>.
Jiawei Wang has uploaded a new patch set (#3). ( http://gerrit.cloudera.org:8080/13801 )

Change subject: IMPALA-5149: Provide query profile in JSON format
......................................................................

IMPALA-5149: Provide query profile in JSON format

Description:
Today there is a text and Thrift version of the query profile, but
it would be useful to have a JSON version for portability and machine
consumption. The ultimate goal is to have a Download JSON format
profile link along with the other two formats.

Modification:
1.Add Json format download option in impala server
2.Add ToJson() function for RuntimeProfile, Counters, EventSequence
3.Add JSON format into QueryStateRecord
3.Add tests for E2E test
3.Modify query profile page to different download option

Tests:
E2E tests:
* tests/webserver/test_web_pages.py - test_download_json_profile

Change-Id: I8181ac818bf22207ca1deabd9220c397ae723ec1
---
M be/src/service/impala-beeswax-server.cc
M be/src/service/impala-hs2-server.cc
M be/src/service/impala-http-handler.cc
M be/src/service/impala-http-handler.h
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/util/runtime-profile-counters.h
M be/src/util/runtime-profile.cc
M be/src/util/runtime-profile.h
M common/thrift/RuntimeProfile.thrift
M tests/webserver/test_web_pages.py
M www/query_profile.tmpl
12 files changed, 410 insertions(+), 26 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/01/13801/3
-- 
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: newpatchset
Gerrit-Change-Id: I8181ac818bf22207ca1deabd9220c397ae723ec1
Gerrit-Change-Number: 13801
Gerrit-PatchSet: 3
Gerrit-Owner: Jiawei Wang <ji...@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: Sahil Takiar <st...@cloudera.com>

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

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13801 )

Change subject: IMPALA-5149: Provide query profile in JSON format
......................................................................


Patch Set 15:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/4754/ DRY_RUN=false


-- 
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: 15
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: Wed, 07 Aug 2019 17:01:49 +0000
Gerrit-HasComments: No

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

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13801 )

Change subject: IMPALA-5149: Provide query profile in JSON format
......................................................................


Patch Set 15: Verified+1


-- 
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: 15
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: Wed, 07 Aug 2019 21:10:11 +0000
Gerrit-HasComments: No

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

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13801 )

Change subject: IMPALA-5149: Provide query profile in JSON format
......................................................................


Patch Set 9:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/3991/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


-- 
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: 9
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: Thu, 25 Jul 2019 03:01:03 +0000
Gerrit-HasComments: No

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

Posted by "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 9:

(23 comments)

http://gerrit.cloudera.org:8080/#/c/13801/9/be/src/service/impala-hs2-server.cc
File be/src/service/impala-hs2-server.cc:

http://gerrit.cloudera.org:8080/#/c/13801/9/be/src/service/impala-hs2-server.cc@988
PS9, Line 988:   }else {
nit: missing blank space after }


http://gerrit.cloudera.org:8080/#/c/13801/9/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:

http://gerrit.cloudera.org:8080/#/c/13801/9/be/src/service/impala-server.cc@101
PS9, Line 101: #include <rapidjson/document.h>
             : #include <rapidjson/error/en.h>
             : #include <rapidjson/stringbuffer.h>
             : #include <rapidjson/writer.h>
Why not group these together with other rapidjson #include above ? There seems to be some duplicates.


http://gerrit.cloudera.org:8080/#/c/13801/9/be/src/service/impala-server.cc@783
PS9, Line 783: DCHECK(format == TRuntimeProfileFormat::STRING);
DCHECK_EQ(format, TRuntimeProfileFormat::STRING);


http://gerrit.cloudera.org:8080/#/c/13801/9/be/src/service/impala-server.cc@818
PS9, Line 818: DCHECK(format == TRuntimeProfileFormat::STRING);
DCHECK_EQ(format, TRuntimeProfileFormat::STRING);


http://gerrit.cloudera.org:8080/#/c/13801/9/be/src/util/runtime-profile-counters.h
File be/src/util/runtime-profile-counters.h:

http://gerrit.cloudera.org:8080/#/c/13801/9/be/src/util/runtime-profile-counters.h@126
PS9, Line 126:   void ToJson(rapidjson::Document& document, rapidjson::Value* val) const {
void ToJson(rapidjson::Document& document, rapidjson::Value* val) const override {


http://gerrit.cloudera.org:8080/#/c/13801/9/be/src/util/runtime-profile-counters.h@131
PS9, Line 131: val->AddMember("current_value", current_value(), document.GetAllocator());
Actually, do you need to output this in json ? It seems sufficient to just call Counter::ToJson() which should add the higher water mark stored in "value_" ? "current_value_" seems to be internal state used for supporting TryAdd() only.


http://gerrit.cloudera.org:8080/#/c/13801/9/be/src/util/runtime-profile-counters.h@165
PS9, Line 165:   void ToJson(rapidjson::Document& document, rapidjson::Value* val) const {
             :     Counter::ToJson(document, val);
             :     rapidjson::Value::MemberIterator type_itr = val->FindMember("kind");
             :     DCHECK(type_itr != val->MemberEnd());
             :     type_itr->value.SetString("DerivedCounter");
             :     val->AddMember("counter_fn_val", counter_fn_(), document.GetAllocator());
             :   }
If you take the suggestion in Counter::ToJson(), you probably don't need this function.


http://gerrit.cloudera.org:8080/#/c/13801/9/be/src/util/runtime-profile-counters.h@219
PS9, Line 219:     rapidjson::Value::MemberIterator type_itr = val->FindMember("kind");
             :     DCHECK(type_itr != val->MemberEnd());
             :     type_itr->value.SetString("AveragedCounter");
See comments in Counter::ToJson() on why we may not need this.


http://gerrit.cloudera.org:8080/#/c/13801/9/be/src/util/runtime-profile-counters.h@222
PS9, Line 222:     if (unit_ == TUnit::type::DOUBLE_VALUE){
             :       val->AddMember("current_double_sum", current_double_sum_, document.GetAllocator());
             :     } else {
             :       val->AddMember("current_int_sum", current_int_sum_, document.GetAllocator());
             :     }
Same question as above. Do you need to store the internal states (i.e. sum) in the json output object ? Isn't "average" the only state we need ?


http://gerrit.cloudera.org:8080/#/c/13801/9/be/src/util/runtime-profile-counters.h@290
PS9, Line 290:   void ToJson(rapidjson::Document& document, rapidjson::Value* val) const {
virtual override


http://gerrit.cloudera.org:8080/#/c/13801/9/be/src/util/runtime-profile-counters.h@292
PS9, Line 292: val->RemoveMember("kind");
Why is "kind" removed ? May be worth adding a comment here.


http://gerrit.cloudera.org:8080/#/c/13801/9/be/src/util/runtime-profile-counters.h@406
PS9, Line 406: void ToJson(rapidjson::Document& document, rapidjson::Value* value);
Please add a comment about the output format.


http://gerrit.cloudera.org:8080/#/c/13801/9/be/src/util/runtime-profile-counters.h@452
PS9, Line 452: void ToJson(rapidjson::Document& document, rapidjson::Value* val);
It'd be nice to add a comment about the output format.


http://gerrit.cloudera.org:8080/#/c/13801/9/be/src/util/runtime-profile-counters.h@583
PS9, Line 583:   void ToJson(rapidjson::Document& document, rapidjson::Value* val) const {
             :     Counter::ToJson(document, val);
             :     rapidjson::Value::MemberIterator type_itr = val->FindMember("kind");
             :     type_itr->value.SetString("ConcurrentTimerCounter");
             :     val->AddMember("concurrent_stop_watch_total_time",
             :         csw_.TotalRunningTime(), document.GetAllocator());
             :   }
Similar to comments above, this may not be needed since Counter::ToJson() may have covered what's needed.


http://gerrit.cloudera.org:8080/#/c/13801/9/be/src/util/runtime-profile.h
File be/src/util/runtime-profile.h:

http://gerrit.cloudera.org:8080/#/c/13801/9/be/src/util/runtime-profile.h@123
PS9, Line 123: name, value, human_readable, description
is this list actually accurate ? What about "kind"  and "unit" ?


http://gerrit.cloudera.org:8080/#/c/13801/9/be/src/util/runtime-profile.cc
File be/src/util/runtime-profile.cc:

http://gerrit.cloudera.org:8080/#/c/13801/9/be/src/util/runtime-profile.cc@709
PS9, Line 709: 
nit: unnecessary blank line


http://gerrit.cloudera.org:8080/#/c/13801/9/be/src/util/runtime-profile.cc@797
PS9, Line 797:     if(!event_sequence_map_.empty()) {
nit: formatting


http://gerrit.cloudera.org:8080/#/c/13801/9/be/src/util/runtime-profile.cc@821
PS9, Line 821:     if(!summary_stats_map_.empty()) {
formatting


http://gerrit.cloudera.org:8080/#/c/13801/9/be/src/util/runtime-profile.cc@855
PS9, Line 855: PrettyPrint() 
incorrect comment from copy-n-paste


http://gerrit.cloudera.org:8080/#/c/13801/9/be/src/util/runtime-profile.cc@1582
PS9, Line 1582: value_.Load()
Use value() instead in case derived class overrides value().


http://gerrit.cloudera.org:8080/#/c/13801/9/be/src/util/runtime-profile.cc@1583
PS9, Line 1583: _TUnit_VALUES_TO_NAMES.find(unit_)
Does it make sense to DCHECK that this cannot be _TUnit_VALUES_TO_NAMES.end() ?


http://gerrit.cloudera.org:8080/#/c/13801/9/be/src/util/runtime-profile.cc@1585
PS9, Line 1585: "Counter"
Instead of just adding "Counter" here, how about we implement a counter type function which returns a counter type function. In this way, the derived class doesn't need to override ToJson(). They only need to override the CounterType() function so that we will get the right counter type here:

   virtual std::string CounterType() const override {
        return "HighWaterMarkCounter";
   }


http://gerrit.cloudera.org:8080/#/c/13801/9/be/src/util/runtime-profile.cc@1594
PS9, Line 1594: _TUnit_VALUES_TO_NAMES.find(unit_)
Same question about DCHECK as above.



-- 
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: 9
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: Thu, 25 Jul 2019 20:03:23 +0000
Gerrit-HasComments: Yes

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

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13801 )

Change subject: IMPALA-5149: Provide query profile in JSON format
......................................................................


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/13801/1/be/src/util/runtime-profile.cc
File be/src/util/runtime-profile.cc:

http://gerrit.cloudera.org:8080/#/c/13801/1/be/src/util/runtime-profile.cc@718
PS1, Line 718:                                          const string& counter_name, const CounterMap& counter_map,
line too long (99 > 90)



-- 
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: 1
Gerrit-Owner: Jiawei Wang <ji...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@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-Comment-Date: Wed, 03 Jul 2019 21:29:52 +0000
Gerrit-HasComments: Yes

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

Posted by "Jiawei Wang (Code Review)" <ge...@cloudera.org>.
Hello Bharath Vissapragada, Greg Rahn, Quanlong Huang, David Rorke, David Knupp, Sahil Takiar, Impala Public Jenkins, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/13801

to look at the new patch set (#7).

Change subject: IMPALA-5149: Provide query profile in JSON format
......................................................................

IMPALA-5149: Provide query profile in JSON format

Description:
Today there is a text and Thrift version of the query profile, but
it would be useful to have a JSON version for portability and machine
consumption. The ultimate goal is to have a Download JSON format
profile link along with the other two formats.

Modification:
1.Add Json format download option in impalad profile page
2.Add ToJson() function for RuntimeProfile, Counters, EventSequence
3.Add JSON format into QueryStateRecord
4.Add tests for E2E test, unit test, hs2 test
5.Modify query profile page to different download option
6.Modify HS2 server to support get JSON format profile

Tests:
E2E tests:
tests/webserver/test_web_pages.py - test_download_profile
 * merge text and json format download together
HS2 tests:
tests/hs2/test_hs2.py - test_get_profile
 * add json format test
BE Unit tests:
be/src/util/runtime-profile-test.cc
 * ToJson.RuntimeProfileToJsonTest
 * ToJson.EmptyTest
 * ToJson.EventSequenceToJsonTest
 * ToJson.TimeSeriesCounterToJsonTest

Change-Id: I8181ac818bf22207ca1deabd9220c397ae723ec1
---
M be/src/service/impala-beeswax-server.cc
M be/src/service/impala-hs2-server.cc
M be/src/service/impala-http-handler.cc
M be/src/service/impala-http-handler.h
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/util/CMakeLists.txt
M be/src/util/runtime-profile-counters.h
M be/src/util/runtime-profile-test.cc
M be/src/util/runtime-profile.cc
M be/src/util/runtime-profile.h
M common/thrift/ImpalaService.thrift
M common/thrift/RuntimeProfile.thrift
M tests/hs2/test_hs2.py
M tests/webserver/test_web_pages.py
M www/query_profile.tmpl
16 files changed, 611 insertions(+), 45 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/01/13801/7
-- 
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: newpatchset
Gerrit-Change-Id: I8181ac818bf22207ca1deabd9220c397ae723ec1
Gerrit-Change-Number: 13801
Gerrit-PatchSet: 7
Gerrit-Owner: Jiawei Wang <ji...@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: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>

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

Posted by "Jiawei Wang (Code Review)" <ge...@cloudera.org>.
Jiawei Wang has posted comments on this change. ( http://gerrit.cloudera.org:8080/13801 )

Change subject: IMPALA-5149: Provide query profile in JSON format
......................................................................


Patch Set 8:

(17 comments)

Thanks so much for the valid feedback. Have adopted the changes you suggested. Also add some more unit tests. Let me know if the code need more changes. Thanks!

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?
Done


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 { .
Done


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)
Done


http://gerrit.cloudera.org:8080/#/c/13801/6/be/src/util/runtime-profile-counters.h@164
PS6, Line 164: 
> same (multiple places below)
Done


http://gerrit.cloudera.org:8080/#/c/13801/6/be/src/util/runtime-profile-counters.h@289
PS6, Line 289: 
> why this?
I am moving this away because SummaryStatsCounter and TimeSeriesCounter are all separate in our output JSON schema. 

“counters”: [
{
	“counter_name”: xxx,
	“value”: xxx,
	“kind”: xxx,
	“unit”: xxx,
	“child_counters”: [{...}]    // same structure as counter
	…  //type special values
},{...}
],
“summary_stats_counters” : [
{
	“counter_name”: xxx,
	“value”: xxx,
	“unit”: xxx,
	“min”: xxx,
	“max”: xxx
	“avg”: xxx
	“num_of_samples”: xxx

},{...}
]

I feel like it is a redundant content if we add its "kind".

What do you think? I can always add it back if you feel that's better.


http://gerrit.cloudera.org:8080/#/c/13801/6/be/src/util/runtime-profile-counters.h@403
PS6, Line 403: 
> move to cc file?
Done


http://gerrit.cloudera.org:8080/#/c/13801/6/be/src/util/runtime-profile-counters.h@421
PS6, Line 421:   EventList events_;
> instantiate value = ...directly in L407?
I am not sure what to do with this... I am using the same schema as metric collection

https://github.com/apache/impala/blob/95a1da2d32ea7b28585ad574b22c2bb9dd921029/be/src/util/collection-metrics.cc#L28


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: 
> check other units too and assert the unit data?
Done


http://gerrit.cloudera.org:8080/#/c/13801/6/be/src/util/runtime-profile-test.cc@1187
PS6, Line 1187:   // Serialize to json
> some tests for info_strings and event_sequences too?
Done


http://gerrit.cloudera.org:8080/#/c/13801/6/be/src/util/runtime-profile-test.cc@1202
PS6, Line 1202:   // Check counter value matches
> check other TimeSeriesCounter implementations too?
Done


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: );
> remove rapidjson classifiers with using...
Done


http://gerrit.cloudera.org:8080/#/c/13801/6/be/src/util/runtime-profile.cc@713
PS6, Line 713:  CounterM
> use d->GetAllocator() (avoid temp variable)
Done


http://gerrit.cloudera.org:8080/#/c/13801/6/be/src/util/runtime-profile.cc@735
PS6, Line 735:   }
> formatting
Done


http://gerrit.cloudera.org:8080/#/c/13801/6/be/src/util/runtime-profile.cc@1590
PS6, Line 1590:   lock_guard<SpinLock> lock(lock_);
> formatting
Done


http://gerrit.cloudera.org:8080/#/c/13801/6/be/src/util/runtime-profile.cc@1600
PS6, Line 1600: 
> formatting
Done


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:     get_profile_req.format = 3  # json format
> why not do this in the above test after L654?
Yeah, I can put this inside the above test.


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:         # the query is in the file.
> merge with test_download_text_profile? You can do something like
Done



-- 
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: 8
Gerrit-Owner: Jiawei Wang <ji...@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: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Comment-Date: Mon, 15 Jul 2019 23:55:48 +0000
Gerrit-HasComments: Yes

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

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/13801 )

Change subject: IMPALA-5149: Provide query profile in JSON format
......................................................................

IMPALA-5149: Provide query profile in JSON format

Description:
Today there is a text and Thrift version of the query profile, but
it would be useful to have a JSON version for portability and machine
consumption. The ultimate goal is to have a Download JSON format
profile link along with the other two formats.

Modification:
1.Add Json format download option in impalad profile page
2.Add ToJson() function for RuntimeProfile, Counters, EventSequence
3.Add JSON format into QueryStateRecord
4.Add tests for E2E test, unit test, hs2 test
5.Modify query profile page to different download option
6.Modify HS2 server to support get JSON format profile

Future compatibility:
The schema of the JSON format can be changed in the future with
the standardization of Profile and Counter structure.

Tests:
E2E tests:
tests/webserver/test_web_pages.py - test_download_profile
 * merge text and json format download together
HS2 tests:
tests/hs2/test_hs2.py - test_get_profile
 * add json format test
BE Unit tests:
be/src/util/runtime-profile-test.cc
 * ToJson.RuntimeProfileToJsonTest
 * ToJson.EmptyTest
 * ToJson.EventSequenceToJsonTest
 * ToJson.TimeSeriesCounterToJsonTest

Change-Id: I8181ac818bf22207ca1deabd9220c397ae723ec1
Reviewed-on: http://gerrit.cloudera.org:8080/13801
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M be/src/service/impala-beeswax-server.cc
M be/src/service/impala-hs2-server.cc
M be/src/service/impala-http-handler.cc
M be/src/service/impala-http-handler.h
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/util/CMakeLists.txt
M be/src/util/runtime-profile-counters.h
M be/src/util/runtime-profile-test.cc
M be/src/util/runtime-profile.cc
M be/src/util/runtime-profile.h
M common/thrift/ImpalaService.thrift
M common/thrift/RuntimeProfile.thrift
M tests/hs2/test_hs2.py
M tests/webserver/test_web_pages.py
M www/query_profile.tmpl
16 files changed, 646 insertions(+), 60 deletions(-)

Approvals:
  Impala Public Jenkins: Looks good to me, approved; Verified

-- 
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: merged
Gerrit-Change-Id: I8181ac818bf22207ca1deabd9220c397ae723ec1
Gerrit-Change-Number: 13801
Gerrit-PatchSet: 16
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>

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

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/4087/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


-- 
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: Tue, 30 Jul 2019 23:32:13 +0000
Gerrit-HasComments: No

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

Posted by "Andrew Sherman (Code Review)" <ge...@cloudera.org>.
Andrew Sherman has posted comments on this change. ( http://gerrit.cloudera.org:8080/13801 )

Change subject: IMPALA-5149: Provide query profile in JSON format
......................................................................


Patch Set 8:

Should the json include the explicit version number of the format?
 "version: "1.0"
to help people writing parsers?
Like Bharath (and DavidR in the design doc) I would like to see more about future compatibility guarantees


-- 
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: 8
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: Mon, 22 Jul 2019 17:50:15 +0000
Gerrit-HasComments: No

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

Posted by "Jiawei Wang (Code Review)" <ge...@cloudera.org>.
Jiawei Wang has uploaded a new patch set (#11). ( http://gerrit.cloudera.org:8080/13801 )

Change subject: IMPALA-5149: Provide query profile in JSON format
......................................................................

IMPALA-5149: Provide query profile in JSON format

Description:
Today there is a text and Thrift version of the query profile, but
it would be useful to have a JSON version for portability and machine
consumption. The ultimate goal is to have a Download JSON format
profile link along with the other two formats.

Modification:
1.Add Json format download option in impalad profile page
2.Add ToJson() function for RuntimeProfile, Counters, EventSequence
3.Add JSON format into QueryStateRecord
4.Add tests for E2E test, unit test, hs2 test
5.Modify query profile page to different download option
6.Modify HS2 server to support get JSON format profile

Future compatibility:
The schema of the JSON format can be changed in the future with
the standardization of Profile and Counter structure.

Tests:
E2E tests:
tests/webserver/test_web_pages.py - test_download_profile
 * merge text and json format download together
HS2 tests:
tests/hs2/test_hs2.py - test_get_profile
 * add json format test
BE Unit tests:
be/src/util/runtime-profile-test.cc
 * ToJson.RuntimeProfileToJsonTest
 * ToJson.EmptyTest
 * ToJson.EventSequenceToJsonTest
 * ToJson.TimeSeriesCounterToJsonTest

Change-Id: I8181ac818bf22207ca1deabd9220c397ae723ec1
---
M be/src/service/impala-beeswax-server.cc
M be/src/service/impala-hs2-server.cc
M be/src/service/impala-http-handler.cc
M be/src/service/impala-http-handler.h
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/util/CMakeLists.txt
M be/src/util/runtime-profile-counters.h
M be/src/util/runtime-profile-test.cc
M be/src/util/runtime-profile.cc
M be/src/util/runtime-profile.h
M common/thrift/ImpalaService.thrift
M common/thrift/RuntimeProfile.thrift
M tests/hs2/test_hs2.py
M tests/webserver/test_web_pages.py
M www/query_profile.tmpl
16 files changed, 642 insertions(+), 60 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/01/13801/11
-- 
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: newpatchset
Gerrit-Change-Id: I8181ac818bf22207ca1deabd9220c397ae723ec1
Gerrit-Change-Number: 13801
Gerrit-PatchSet: 11
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>

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

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13801 )

Change subject: IMPALA-5149: Provide query profile in JSON format
......................................................................


Patch Set 5:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/13801/5/tests/webserver/test_web_pages.py
File tests/webserver/test_web_pages.py:

http://gerrit.cloudera.org:8080/#/c/13801/5/tests/webserver/test_web_pages.py@608
PS5, Line 608: e
flake8: E722 do not use bare except'



-- 
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: 5
Gerrit-Owner: Jiawei Wang <ji...@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: Sahil Takiar <st...@cloudera.com>
Gerrit-Comment-Date: Wed, 10 Jul 2019 01:42:23 +0000
Gerrit-HasComments: Yes

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

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13801 )

Change subject: IMPALA-5149: Provide query profile in JSON format
......................................................................


Patch Set 7:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/3884/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


-- 
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: 7
Gerrit-Owner: Jiawei Wang <ji...@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: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Comment-Date: Tue, 16 Jul 2019 00:30:12 +0000
Gerrit-HasComments: No

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

Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
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 8: Code-Review+1

(5 comments)

Some minor comments, but generally looks good to me. I spoke to Michael Ho offline who is going to take another look into the change and take it to a +2. Thanks for your patience so far.

http://gerrit.cloudera.org:8080/#/c/13801/8//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/13801/8//COMMIT_MSG@22
PS8, Line 22: 
Add a section on future compatibility guarantees?

I think the idea is that this can evolve over time until we standardize the counters and the profile structure. So, until then if there are some Impal a changes, clients can expect the JSON structure to change too (keys could be added, deleted etc.)


http://gerrit.cloudera.org:8080/#/c/13801/8/be/src/util/runtime-profile-counters.h
File be/src/util/runtime-profile-counters.h:

http://gerrit.cloudera.org:8080/#/c/13801/8/be/src/util/runtime-profile-counters.h@292
PS8, Line 292:     val->RemoveMember("kind");
Should we not update the kind ? Why remove it?


http://gerrit.cloudera.org:8080/#/c/13801/8/tests/hs2/test_hs2.py
File tests/hs2/test_hs2.py:

http://gerrit.cloudera.org:8080/#/c/13801/8/tests/hs2/test_hs2.py@668
PS8, Line 668: json_res["contents"]["child_profiles"][0]["info_strings"]:
This looks flaky. Instead assert that they exist before you can loop through them?


http://gerrit.cloudera.org:8080/#/c/13801/8/tests/hs2/test_hs2.py@670
PS8, Line 670:         assert statement in info_string["value"]
dump the json_res if the assert fails, helps with debugging test failures.


http://gerrit.cloudera.org:8080/#/c/13801/8/tests/webserver/test_web_pages.py
File tests/webserver/test_web_pages.py:

http://gerrit.cloudera.org:8080/#/c/13801/8/tests/webserver/test_web_pages.py@594
PS8, Line 594:           self.fail("Download JSON format query profile cannot be parsed.")
dump 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: 8
Gerrit-Owner: Jiawei Wang <ji...@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: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Comment-Date: Fri, 19 Jul 2019 17:20:25 +0000
Gerrit-HasComments: Yes

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

Posted by "Jiawei Wang (Code Review)" <ge...@cloudera.org>.
Jiawei Wang has uploaded a new patch set (#13). ( http://gerrit.cloudera.org:8080/13801 )

Change subject: IMPALA-5149: Provide query profile in JSON format
......................................................................

IMPALA-5149: Provide query profile in JSON format

Description:
Today there is a text and Thrift version of the query profile, but
it would be useful to have a JSON version for portability and machine
consumption. The ultimate goal is to have a Download JSON format
profile link along with the other two formats.

Modification:
1.Add Json format download option in impalad profile page
2.Add ToJson() function for RuntimeProfile, Counters, EventSequence
3.Add JSON format into QueryStateRecord
4.Add tests for E2E test, unit test, hs2 test
5.Modify query profile page to different download option
6.Modify HS2 server to support get JSON format profile

Future compatibility:
The schema of the JSON format can be changed in the future with
the standardization of Profile and Counter structure.

Tests:
E2E tests:
tests/webserver/test_web_pages.py - test_download_profile
 * merge text and json format download together
HS2 tests:
tests/hs2/test_hs2.py - test_get_profile
 * add json format test
BE Unit tests:
be/src/util/runtime-profile-test.cc
 * ToJson.RuntimeProfileToJsonTest
 * ToJson.EmptyTest
 * ToJson.EventSequenceToJsonTest
 * ToJson.TimeSeriesCounterToJsonTest

Change-Id: I8181ac818bf22207ca1deabd9220c397ae723ec1
---
M be/src/service/impala-beeswax-server.cc
M be/src/service/impala-hs2-server.cc
M be/src/service/impala-http-handler.cc
M be/src/service/impala-http-handler.h
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/util/CMakeLists.txt
M be/src/util/runtime-profile-counters.h
M be/src/util/runtime-profile-test.cc
M be/src/util/runtime-profile.cc
M be/src/util/runtime-profile.h
M common/thrift/ImpalaService.thrift
M common/thrift/RuntimeProfile.thrift
M tests/hs2/test_hs2.py
M tests/webserver/test_web_pages.py
M www/query_profile.tmpl
16 files changed, 649 insertions(+), 60 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/01/13801/13
-- 
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: newpatchset
Gerrit-Change-Id: I8181ac818bf22207ca1deabd9220c397ae723ec1
Gerrit-Change-Number: 13801
Gerrit-PatchSet: 13
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>

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

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13801 )

Change subject: IMPALA-5149: Provide query profile in JSON format
......................................................................


Patch Set 4:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/3853/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


-- 
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: 4
Gerrit-Owner: Jiawei Wang <ji...@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: Sahil Takiar <st...@cloudera.com>
Gerrit-Comment-Date: Wed, 10 Jul 2019 02:16:36 +0000
Gerrit-HasComments: No

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

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13801 )

Change subject: IMPALA-5149: Provide query profile in JSON format
......................................................................


Patch Set 2:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/3838/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


-- 
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: 2
Gerrit-Owner: Jiawei Wang <ji...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@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-Comment-Date: Mon, 08 Jul 2019 18:12:44 +0000
Gerrit-HasComments: No

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

Posted by "Jiawei Wang (Code Review)" <ge...@cloudera.org>.
Jiawei Wang 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:

(1 comment)

Thanks for your feedback! I also added some tests for this.

http://gerrit.cloudera.org:8080/#/c/13801/2/be/src/service/impala-hs2-server.cc
File be/src/service/impala-hs2-server.cc:

http://gerrit.cloudera.org:8080/#/c/13801/2/be/src/service/impala-hs2-server.cc@979
PS2, Line 979:       SQLSTATE_GENERAL_ERROR);
             :   if (request.format == TRuntimeProfileFormat::THRIFT) {
             :     return_val.__set_thrift_profile(thrift_profile);
             :   } else if (request.format == TRuntimeProfileFormat::JSON) {
             :     rapidjson::StringBuffer sb;
             :     rapidjson::PrettyWriter<rapidjson::StringBuffer> writer(sb);
             :    
> should there be an equivalent here for JSON?
Done. I am putting the output inside the profile, which is a string.

Also, do we need to adopt this change to impala-beeswax-server? I looked at the code there but find that the function there only support text format download.



-- 
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 <ji...@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: Sahil Takiar <st...@cloudera.com>
Gerrit-Comment-Date: Thu, 11 Jul 2019 00:05:20 +0000
Gerrit-HasComments: Yes

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

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13801 )

Change subject: IMPALA-5149: Provide query profile in JSON format
......................................................................


Patch Set 10:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/4034/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


-- 
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: 10
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, 26 Jul 2019 21:34:27 +0000
Gerrit-HasComments: No

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

Posted by "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

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

Posted by "Jiawei Wang (Code Review)" <ge...@cloudera.org>.
Jiawei Wang has uploaded a new patch set (#10). ( http://gerrit.cloudera.org:8080/13801 )

Change subject: IMPALA-5149: Provide query profile in JSON format
......................................................................

IMPALA-5149: Provide query profile in JSON format

Description:
Today there is a text and Thrift version of the query profile, but
it would be useful to have a JSON version for portability and machine
consumption. The ultimate goal is to have a Download JSON format
profile link along with the other two formats.

Modification:
1.Add Json format download option in impalad profile page
2.Add ToJson() function for RuntimeProfile, Counters, EventSequence
3.Add JSON format into QueryStateRecord
4.Add tests for E2E test, unit test, hs2 test
5.Modify query profile page to different download option
6.Modify HS2 server to support get JSON format profile

Future compatibility:
The schema of the JSON format can be changed in the future with
the standardization of Profile and Counter structure.

Tests:
E2E tests:
tests/webserver/test_web_pages.py - test_download_profile
 * merge text and json format download together
HS2 tests:
tests/hs2/test_hs2.py - test_get_profile
 * add json format test
BE Unit tests:
be/src/util/runtime-profile-test.cc
 * ToJson.RuntimeProfileToJsonTest
 * ToJson.EmptyTest
 * ToJson.EventSequenceToJsonTest
 * ToJson.TimeSeriesCounterToJsonTest

Change-Id: I8181ac818bf22207ca1deabd9220c397ae723ec1
---
M be/src/service/impala-beeswax-server.cc
M be/src/service/impala-hs2-server.cc
M be/src/service/impala-http-handler.cc
M be/src/service/impala-http-handler.h
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/util/CMakeLists.txt
M be/src/util/runtime-profile-counters.h
M be/src/util/runtime-profile-test.cc
M be/src/util/runtime-profile.cc
M be/src/util/runtime-profile.h
M common/thrift/ImpalaService.thrift
M common/thrift/RuntimeProfile.thrift
M tests/hs2/test_hs2.py
M tests/webserver/test_web_pages.py
M www/query_profile.tmpl
16 files changed, 642 insertions(+), 60 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/01/13801/10
-- 
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: newpatchset
Gerrit-Change-Id: I8181ac818bf22207ca1deabd9220c397ae723ec1
Gerrit-Change-Number: 13801
Gerrit-PatchSet: 10
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>

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

Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
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 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/13801/2/be/src/service/impala-hs2-server.cc
File be/src/service/impala-hs2-server.cc:

http://gerrit.cloudera.org:8080/#/c/13801/2/be/src/service/impala-hs2-server.cc@979
PS2, Line 979:   if (request.format == TRuntimeProfileFormat::THRIFT) {
             :     return_val.__set_thrift_profile(thrift_profile);
             :   } else {
             :     DCHECK(request.format == TRuntimeProfileFormat::STRING
             :         || request.format == TRuntimeProfileFormat::BASE64);
             :     return_val.__set_profile(ss.str());
             :   }
> not sure; I'm not too familiar with this part of the code, I guess it depen
I don't think we need to fix this for beeswax server as it will be deprecated in the future, especially given that shell now supports HS2 protocol.



-- 
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: 2
Gerrit-Owner: Jiawei Wang <ji...@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: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Comment-Date: Mon, 15 Jul 2019 22:43:44 +0000
Gerrit-HasComments: Yes

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

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13801 )

Change subject: IMPALA-5149: Provide query profile in JSON format
......................................................................


Patch Set 14:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/4162/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


-- 
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: 14
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: Tue, 06 Aug 2019 20:53:25 +0000
Gerrit-HasComments: No

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

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13801 )

Change subject: IMPALA-5149: Provide query profile in JSON format
......................................................................


Patch Set 5:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/3854/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


-- 
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: 5
Gerrit-Owner: Jiawei Wang <ji...@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: Sahil Takiar <st...@cloudera.com>
Gerrit-Comment-Date: Wed, 10 Jul 2019 02:22:53 +0000
Gerrit-HasComments: No

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

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13801 )

Change subject: IMPALA-5149: Provide query profile in JSON format
......................................................................


Patch Set 8:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/3885/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


-- 
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: 8
Gerrit-Owner: Jiawei Wang <ji...@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: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Comment-Date: Tue, 16 Jul 2019 00:31:47 +0000
Gerrit-HasComments: No

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

Posted by "Jiawei Wang (Code Review)" <ge...@cloudera.org>.
Jiawei Wang has posted comments on this change. ( http://gerrit.cloudera.org:8080/13801 )

Change subject: IMPALA-5149: Provide query profile in JSON format
......................................................................


Patch Set 14:

(14 comments)

> Uploaded patch set 14.

This has been a long process, thanks for your patience on this!

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: Of
> offset: $1
Done


http://gerrit.cloudera.org:8080/#/c/13801/13/be/src/service/impala-server.cc@2026
PS13, Line 2026: json_profile(rapidj
> json_profile. No need for "_rapid".
Done


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_ab = RuntimeProfile::Create(&pool, "ProfileAb");
> The name of the profile should probably match the variable name.
Done


http://gerrit.cloudera.org:8080/#/c/13801/13/be/src/util/runtime-profile-test.cc@1204
PS13, Line 1204:   for (auto& itr : content["counters"].GetArray()) {
               :     // check normal Counter
> Use iterator style like the following. Same at other places.
Done


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 ?
Done


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: A map of counters name to counter
> A map of counter's name to counter
Done


http://gerrit.cloudera.org:8080/#/c/13801/13/be/src/util/runtime-profile.h@625
PS13, Line 625: ame, const Counte
> This needs to be documented too.
Done


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: e information
> What does all the other information mean ?
Done


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)
              :         assert down
> nit: one line
Done


http://gerrit.cloudera.org:8080/#/c/13801/13/tests/webserver/test_web_pages.py@587
PS13, Line 587: 
> nit: indent 4 for line continuation
Done


http://gerrit.cloudera.org:8080/#/c/13801/13/tests/webserver/test_web_pages.py@594
PS13, Line 594: le:{0}".
> Downloaded ?
Done


http://gerrit.cloudera.org:8080/#/c/13801/13/tests/webserver/test_web_pages.py@595
PS13, Line 595: the query id 
> Json pofile
Done


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"], json_res to help d
Done


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: (Available Form
> (available formats):
Done



-- 
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: 14
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: Tue, 06 Aug 2019 20:12:47 +0000
Gerrit-HasComments: Yes

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

Posted by "Jiawei Wang (Code Review)" <ge...@cloudera.org>.
Jiawei Wang has posted comments on this change. ( http://gerrit.cloudera.org:8080/13801 )

Change subject: IMPALA-5149: Provide query profile in JSON format
......................................................................


Patch Set 9:

(4 comments)

Done, thanks for the feedback. I think we need more discussion on the version control thing as Andrew and Bharath mentioned.

http://gerrit.cloudera.org:8080/#/c/13801/8/be/src/util/runtime-profile-counters.h
File be/src/util/runtime-profile-counters.h:

http://gerrit.cloudera.org:8080/#/c/13801/8/be/src/util/runtime-profile-counters.h@292
PS8, Line 292:     val->RemoveMember("kind");
> Should we not update the kind ? Why remove it?
SummaryStatsCounter and TimeSeriesCounter are treated separately with normal counters in our JSON output. So a field like "kind" would be redundant.

“counters”: [
{
	“counter_name”: xxx,
	“value”: xxx,
	“kind”: xxx,
	“unit”: xxx,
	“child_counters”: [{...}]    // same structure as counter
	…  //type special values
},{...}
],
“summary_stats_counters” : [
{
		“counter_name”: xxx,
	“value”: xxx,
	“unit”: xxx,
	“min”: xxx,
	“max”: xxx
	“avg”: xxx
	“num_of_samples”: xxx

},{...}
]

The difference as you can see from the schema, it's that we already know all the counters from summary_stats_counters belongs to SummaryStatsCounter. So it would not be necessary to keep the "kind" field again.


http://gerrit.cloudera.org:8080/#/c/13801/8/tests/hs2/test_hs2.py
File tests/hs2/test_hs2.py:

http://gerrit.cloudera.org:8080/#/c/13801/8/tests/hs2/test_hs2.py@668
PS8, Line 668: 
> This looks flaky. Instead assert that they exist before you can loop throug
Done


http://gerrit.cloudera.org:8080/#/c/13801/8/tests/hs2/test_hs2.py@670
PS8, Line 670:     if ("child_profiles" not in json_res["contents"]) or \
> dump the json_res if the assert fails, helps with debugging test failures.
Done


http://gerrit.cloudera.org:8080/#/c/13801/8/tests/webserver/test_web_pages.py
File tests/webserver/test_web_pages.py:

http://gerrit.cloudera.org:8080/#/c/13801/8/tests/webserver/test_web_pages.py@594
PS8, Line 594:           assert False, "Download JSON format query profile cannot be parsed. " \
> dump json
Done



-- 
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: 9
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: Thu, 25 Jul 2019 02:20:35 +0000
Gerrit-HasComments: Yes

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

Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
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 <ji...@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: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Comment-Date: Sat, 13 Jul 2019 04:00:03 +0000
Gerrit-HasComments: Yes

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

Posted by "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 14: Code-Review+2


-- 
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: 14
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: Wed, 07 Aug 2019 17:01:00 +0000
Gerrit-HasComments: No

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

Posted by "Jiawei Wang (Code Review)" <ge...@cloudera.org>.
Jiawei Wang has uploaded a new patch set (#14). ( http://gerrit.cloudera.org:8080/13801 )

Change subject: IMPALA-5149: Provide query profile in JSON format
......................................................................

IMPALA-5149: Provide query profile in JSON format

Description:
Today there is a text and Thrift version of the query profile, but
it would be useful to have a JSON version for portability and machine
consumption. The ultimate goal is to have a Download JSON format
profile link along with the other two formats.

Modification:
1.Add Json format download option in impalad profile page
2.Add ToJson() function for RuntimeProfile, Counters, EventSequence
3.Add JSON format into QueryStateRecord
4.Add tests for E2E test, unit test, hs2 test
5.Modify query profile page to different download option
6.Modify HS2 server to support get JSON format profile

Future compatibility:
The schema of the JSON format can be changed in the future with
the standardization of Profile and Counter structure.

Tests:
E2E tests:
tests/webserver/test_web_pages.py - test_download_profile
 * merge text and json format download together
HS2 tests:
tests/hs2/test_hs2.py - test_get_profile
 * add json format test
BE Unit tests:
be/src/util/runtime-profile-test.cc
 * ToJson.RuntimeProfileToJsonTest
 * ToJson.EmptyTest
 * ToJson.EventSequenceToJsonTest
 * ToJson.TimeSeriesCounterToJsonTest

Change-Id: I8181ac818bf22207ca1deabd9220c397ae723ec1
---
M be/src/service/impala-beeswax-server.cc
M be/src/service/impala-hs2-server.cc
M be/src/service/impala-http-handler.cc
M be/src/service/impala-http-handler.h
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/util/CMakeLists.txt
M be/src/util/runtime-profile-counters.h
M be/src/util/runtime-profile-test.cc
M be/src/util/runtime-profile.cc
M be/src/util/runtime-profile.h
M common/thrift/ImpalaService.thrift
M common/thrift/RuntimeProfile.thrift
M tests/hs2/test_hs2.py
M tests/webserver/test_web_pages.py
M www/query_profile.tmpl
16 files changed, 646 insertions(+), 60 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/01/13801/14
-- 
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: newpatchset
Gerrit-Change-Id: I8181ac818bf22207ca1deabd9220c397ae723ec1
Gerrit-Change-Number: 13801
Gerrit-PatchSet: 14
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>

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

Posted by "Sahil Takiar (Code Review)" <ge...@cloudera.org>.
Sahil Takiar has posted comments on this change. ( http://gerrit.cloudera.org:8080/13801 )

Change subject: IMPALA-5149: Provide query profile in JSON format
......................................................................


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/13801/2/be/src/service/impala-hs2-server.cc
File be/src/service/impala-hs2-server.cc:

http://gerrit.cloudera.org:8080/#/c/13801/2/be/src/service/impala-hs2-server.cc@979
PS2, Line 979:   if (request.format == TRuntimeProfileFormat::THRIFT) {
             :     return_val.__set_thrift_profile(thrift_profile);
             :   } else {
             :     DCHECK(request.format == TRuntimeProfileFormat::STRING
             :         || request.format == TRuntimeProfileFormat::BASE64);
             :     return_val.__set_profile(ss.str());
             :   }
> Done. I am putting the output inside the profile, which is a string.
not sure; I'm not too familiar with this part of the code, I guess it depends on when this method is invoked vs. the equivalent in impala-beeswax-server



-- 
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: 2
Gerrit-Owner: Jiawei Wang <ji...@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: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Comment-Date: Mon, 15 Jul 2019 20:37:17 +0000
Gerrit-HasComments: Yes

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

Posted by "Jiawei Wang (Code Review)" <ge...@cloudera.org>.
Jiawei Wang has uploaded a new patch set (#9). ( http://gerrit.cloudera.org:8080/13801 )

Change subject: IMPALA-5149: Provide query profile in JSON format
......................................................................

IMPALA-5149: Provide query profile in JSON format

Description:
Today there is a text and Thrift version of the query profile, but
it would be useful to have a JSON version for portability and machine
consumption. The ultimate goal is to have a Download JSON format
profile link along with the other two formats.

Modification:
1.Add Json format download option in impalad profile page
2.Add ToJson() function for RuntimeProfile, Counters, EventSequence
3.Add JSON format into QueryStateRecord
4.Add tests for E2E test, unit test, hs2 test
5.Modify query profile page to different download option
6.Modify HS2 server to support get JSON format profile

Tests:
E2E tests:
tests/webserver/test_web_pages.py - test_download_profile
 * merge text and json format download together
HS2 tests:
tests/hs2/test_hs2.py - test_get_profile
 * add json format test
BE Unit tests:
be/src/util/runtime-profile-test.cc
 * ToJson.RuntimeProfileToJsonTest
 * ToJson.EmptyTest
 * ToJson.EventSequenceToJsonTest
 * ToJson.TimeSeriesCounterToJsonTest

Change-Id: I8181ac818bf22207ca1deabd9220c397ae723ec1
---
M be/src/service/impala-beeswax-server.cc
M be/src/service/impala-hs2-server.cc
M be/src/service/impala-http-handler.cc
M be/src/service/impala-http-handler.h
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/util/CMakeLists.txt
M be/src/util/runtime-profile-counters.h
M be/src/util/runtime-profile-test.cc
M be/src/util/runtime-profile.cc
M be/src/util/runtime-profile.h
M common/thrift/ImpalaService.thrift
M common/thrift/RuntimeProfile.thrift
M tests/hs2/test_hs2.py
M tests/webserver/test_web_pages.py
M www/query_profile.tmpl
16 files changed, 620 insertions(+), 45 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/01/13801/9
-- 
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: newpatchset
Gerrit-Change-Id: I8181ac818bf22207ca1deabd9220c397ae723ec1
Gerrit-Change-Number: 13801
Gerrit-PatchSet: 9
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>

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

Posted by "Jiawei Wang (Code Review)" <ge...@cloudera.org>.
Hello Bharath Vissapragada, Greg Rahn, David Rorke, David Knupp, Sahil Takiar, Impala Public Jenkins, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/13801

to look at the new patch set (#6).

Change subject: IMPALA-5149: Provide query profile in JSON format
......................................................................

IMPALA-5149: Provide query profile in JSON format

Description:
Today there is a text and Thrift version of the query profile, but
it would be useful to have a JSON version for portability and machine
consumption. The ultimate goal is to have a Download JSON format
profile link along with the other two formats.

Modification:
1.Add Json format download option in impalad profile page
2.Add ToJson() function for RuntimeProfile, Counters, EventSequence
3.Add JSON format into QueryStateRecord
4.Add tests for E2E test, unit test, hs2 test
5.Modify query profile page to different download option
6.Modify HS2 server to support get JSON format profile

Tests:
E2E tests:
* tests/webserver/test_web_pages.py - test_download_json_profile
HS2 tests:
* tests/hs2/test_hs2.py - test_get_profile_json
BE Unit tests:
* be/src/util/runtime-profile-test.cc
    ToJson.RuntimeProfileToJsonTest
    ToJson.TimeSeriesCounterToJsonTest

Change-Id: I8181ac818bf22207ca1deabd9220c397ae723ec1
---
M be/src/service/impala-beeswax-server.cc
M be/src/service/impala-hs2-server.cc
M be/src/service/impala-http-handler.cc
M be/src/service/impala-http-handler.h
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/util/CMakeLists.txt
M be/src/util/runtime-profile-counters.h
M be/src/util/runtime-profile-test.cc
M be/src/util/runtime-profile.cc
M be/src/util/runtime-profile.h
M common/thrift/ImpalaService.thrift
M common/thrift/RuntimeProfile.thrift
M tests/hs2/test_hs2.py
M tests/webserver/test_web_pages.py
M www/query_profile.tmpl
16 files changed, 525 insertions(+), 30 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/01/13801/6
-- 
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: newpatchset
Gerrit-Change-Id: I8181ac818bf22207ca1deabd9220c397ae723ec1
Gerrit-Change-Number: 13801
Gerrit-PatchSet: 6
Gerrit-Owner: Jiawei Wang <ji...@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: Sahil Takiar <st...@cloudera.com>