Sahil Takiar has posted comments on this change. ( http://gerrit.cloudera.org:8080/13333 )
Change subject: IMPALA-6903: Download profile from WebUI in text format ...................................................................... Patch Set 3: (9 comments) http://gerrit.cloudera.org:8080/#/c/13333/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/13333/3//COMMIT_MSG@10 PS3, Line 10: The link allows users to download runtime profiles as UTF-8 encoded file. nit: this line should be wrapped http://gerrit.cloudera.org:8080/#/c/13333/3//COMMIT_MSG@14 PS3, Line 14: Add test_download_text_profile to test_web_pages.py Typically you want to include something like "ran all core tests" http://gerrit.cloudera.org:8080/#/c/13333/3/be/src/service/impala-http-handler.h File be/src/service/impala-http-handler.h: http://gerrit.cloudera.org:8080/#/c/13333/3/be/src/service/impala-http-handler.h@119 PS3, Line 119: void QueryProfilePlainTextHandler(const Webserver::WebRequest& req, nit: not sure if "Plain" is the correct descriptor here, its a big vague what "Plain" means exactly. I would suggest either just saying "Text" or saying "UTF8Text" http://gerrit.cloudera.org:8080/#/c/13333/3/be/src/service/impala-http-handler.cc File be/src/service/impala-http-handler.cc: http://gerrit.cloudera.org:8080/#/c/13333/3/be/src/service/impala-http-handler.cc@127 PS3, Line 127: webserver->RegisterUrlCallback("/query_profile_plain_text", "raw_text.tmpl", same here, not really sure what "plain" means http://gerrit.cloudera.org:8080/#/c/13333/3/be/src/service/impala-http-handler.cc@265 PS3, Line 265: void ImpalaHttpHandler::QueryProfilePlainTextHandler(const Webserver::WebRequest& req, Is there any real difference between this method and the one above, besides using TRuntimeProfileFormat::STRING vs. TRuntimeProfileFormat::BASE64? Can we just create a new helper method, that both these methods delegate to. The new helper method can take in a TRuntimeProfileFormat type that is passed into GetRuntimeProfileOutput. http://gerrit.cloudera.org:8080/#/c/13333/3/tests/webserver/test_web_pages.py File tests/webserver/test_web_pages.py: http://gerrit.cloudera.org:8080/#/c/13333/3/tests/webserver/test_web_pages.py@551 PS3, Line 551: while not self.client.state_is_finished(query_handle): Use self.wait_for_state(query_handle, [state], [timeout]) http://gerrit.cloudera.org:8080/#/c/13333/3/tests/webserver/test_web_pages.py@554 PS3, Line 554: profile_page_url = "{0}query_profile?query_id={1}".format( this seems a little odd, especially because self.ROOT_URL requires formatting as well, is there a better way to express this http://gerrit.cloudera.org:8080/#/c/13333/3/tests/webserver/test_web_pages.py@563 PS3, Line 563: assert download_link in responses[0].text do we need a null check here - e.g. if responses[0].text is null http://gerrit.cloudera.org:8080/#/c/13333/3/tests/webserver/test_web_pages.py@568 PS3, Line 568: self.ROOT_URL + download_link, query, self.IMPALAD_TEST_PORT) self.ROOT_URL.format(download_link) -- To view, visit http://gerrit.cloudera.org:8080/13333 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie030c2bb330211f51840417b9f7880f19174af7b Gerrit-Change-Number: 13333 Gerrit-PatchSet: 3 Gerrit-Owner: Yongzhi Chen <yc...@cloudera.com> Gerrit-Reviewer: Fredy Wijaya <fwij...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Lars Volker <l...@cloudera.com> Gerrit-Reviewer: Sahil Takiar <stak...@cloudera.com> Gerrit-Reviewer: Yongzhi Chen <yc...@cloudera.com> Gerrit-Comment-Date: Tue, 21 May 2019 07:03:22 +0000 Gerrit-HasComments: Yes
