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 <[email protected]> Gerrit-Reviewer: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: David Knupp <[email protected]> Gerrit-Reviewer: David Rorke <[email protected]> Gerrit-Reviewer: Greg Rahn <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Comment-Date: Tue, 09 Jul 2019 06:21:45 +0000 Gerrit-HasComments: Yes
