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 1: (7 comments) http://gerrit.cloudera.org:8080/#/c/13333/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/13333/1//COMMIT_MSG@9 PS1, Line 9: Get query profile string and create URL to download Suggest adding a few more things; mention that the profile tab now includes a link called "Download Profile" that allows users to download runtime profiles as UTF-8 encoded strings. I would document the technical approach a bit more here as well. Reading through the code, it looks like rather than making a separate HTTP request to the backend for the profile, this change just takes the runtime profile that has already been loaded onto the web-page, and just allows users to download it as a text file. I would suggest adding a few screenshots to the JIRA of the changes as well. http://gerrit.cloudera.org:8080/#/c/13333/1/tests/webserver/test_web_pages.py File tests/webserver/test_web_pages.py: http://gerrit.cloudera.org:8080/#/c/13333/1/tests/webserver/test_web_pages.py@546 PS1, Line 546: def test_download_textprofile(self): should we add another test similar to test_query_profile_encoded_unknown_query_id? or update test_query_profile_encoded_unknown_query_id to do the same thing for text profiles nit: test_download_textprofile --> test_download_text_profile http://gerrit.cloudera.org:8080/#/c/13333/1/tests/webserver/test_web_pages.py@549 PS1, Line 549: query_handle = self.client.execute_async(query) is there a race condition if this is executed asynchronously? what if the runtime profile is not ready by the time it is requested below? http://gerrit.cloudera.org:8080/#/c/13333/1/tests/webserver/test_web_pages.py@555 PS1, Line 555: assert len(responses) == 1 > should we have more assert instead of just asserting the len? +1 I think we could add some asserts on the response data to make sure we are getting back a valid text profile for the given query. I think test_admission_controller.py has some examples of parsing runtime profiles. http://gerrit.cloudera.org:8080/#/c/13333/1/www/query_profile.tmpl File www/query_profile.tmpl: http://gerrit.cloudera.org:8080/#/c/13333/1/www/query_profile.tmpl@34 PS1, Line 34: <a id="txtprofile" href="javascript:void(0)" download="profileutf8_{{query_id}}"> is download the name of the file that will be downloaded? if so, I suggest renaming the file to 'profile_{{query_id}}' and changing the thrift encoded version to 'thrift_profile_{{query_id}}'. Not sure how others feel, but I think that makes more sense. http://gerrit.cloudera.org:8080/#/c/13333/1/www/query_profile.tmpl@43 PS1, Line 43: var contents = document.getElementsByTagName("pre")[0].innerText; I think it would make more sense to use getElementById instead of getElementsByTagName, similar to what you have done for the txtprofile http://gerrit.cloudera.org:8080/#/c/13333/1/www/query_profile.tmpl@49 PS1, Line 49: var dl = document.getElementById("txtprofile"); nit: txtprofile -> text_profile -- 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: 1 Gerrit-Owner: Yongzhi Chen <[email protected]> Gerrit-Reviewer: Fredy Wijaya <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Reviewer: Sahil Takiar <[email protected]> Gerrit-Comment-Date: Tue, 14 May 2019 22:38:34 +0000 Gerrit-HasComments: Yes
