Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14782 )
Change subject: IMPALA-9182: Print the socket address of the client closing a session or cancelling a query from the WebUI ...................................................................... Patch Set 4: (5 comments) Looks good overall, had a few style comments http://gerrit.cloudera.org:8080/#/c/14782/4/be/src/kudu/util/web_callback_registry.h File be/src/kudu/util/web_callback_registry.h: http://gerrit.cloudera.org:8080/#/c/14782/4/be/src/kudu/util/web_callback_registry.h@65 PS4, Line 65: // Socket address of the source of the request Might be worth documenting what the string looks like, e.g. is it "host:port"? http://gerrit.cloudera.org:8080/#/c/14782/4/be/src/service/impala-http-handler.cc File be/src/service/impala-http-handler.cc: http://gerrit.cloudera.org:8080/#/c/14782/4/be/src/service/impala-http-handler.cc@216 PS4, Line 216: c << "Cancelled from Impala's debug web interface by client at " << req.source_socket; Can you use Substitute() instead? We generally prefer that over stringstream. http://gerrit.cloudera.org:8080/#/c/14782/4/be/src/service/impala-http-handler.cc@217 PS4, Line 217: .c_str() .c_str() shouldn't be needed, Status can take a std::string in its constructor. http://gerrit.cloudera.org:8080/#/c/14782/4/be/src/service/impala-http-handler.cc@239 PS4, Line 239: stringstream c; Same comments as above. http://gerrit.cloudera.org:8080/#/c/14782/4/tests/webserver/test_web_pages.py File tests/webserver/test_web_pages.py: http://gerrit.cloudera.org:8080/#/c/14782/4/tests/webserver/test_web_pages.py@733 PS4, Line 733: 10000000 Can you make this shorter? Say 30s at most. The problem is that the way sleep() is implemented the query can get stuck for quite a while and interfere with other tests. -- To view, visit http://gerrit.cloudera.org:8080/14782 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Icf74ad06ce1c40fab4ce37de6b7ca78e3e520b43 Gerrit-Change-Number: 14782 Gerrit-PatchSet: 4 Gerrit-Owner: Vincent Tran <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Mon, 25 Nov 2019 22:27:45 +0000 Gerrit-HasComments: Yes
