Jason Fehr has posted comments on this change. ( http://gerrit.cloudera.org:8080/19428 )
Change subject: IMPALA-11850 Adds HTTP tracing headers when using the hs2-http protocol. ...................................................................... Patch Set 7: (5 comments) addressed comments http://gerrit.cloudera.org:8080/#/c/19428/6/be/src/transport/THttpServer.cpp File be/src/transport/THttpServer.cpp: http://gerrit.cloudera.org:8080/#/c/19428/6/be/src/transport/THttpServer.cpp@249 PS6, Line 249: VLOG_RPC << "HTTP Connection Tracing Headers" > I think VLOG_QUERY is VLOG(1) I switched to VLOG_RPC which is VLOG(2). I don't have an opinion either way, so let's start with a higher log level and drop it down if requested by the Impala community. http://gerrit.cloudera.org:8080/#/c/19428/6/be/src/transport/THttpServer.cpp@250 PS6, Line 250: << (header_x_request_id_.empty() ? "" : " x-request-id=" + header_x_request_id_) > I think the log output should also be x-request-id (or similar). This makes I like this suggestion. It makes things clearer. Will make the change. http://gerrit.cloudera.org:8080/#/c/19428/6/shell/impala_client.py File shell/impala_client.py: http://gerrit.cloudera.org:8080/#/c/19428/6/shell/impala_client.py@a816 PS6, Line 816: > Was this line deliberately removed? Yes. It was unnecessary because the finally block called this same function. Deleting the line removed duplicate function calls. http://gerrit.cloudera.org:8080/#/c/19428/6/shell/impala_shell.py File shell/impala_shell.py: http://gerrit.cloudera.org:8080/#/c/19428/6/shell/impala_shell.py@266 PS6, Line 266: self.http_tracing = not options.no_http_tracing > I think it would be better to have the command line option to be just "http By default, the Impala shell includes the http tracing headers. This option enables users to remove the http tracing headers if they want. I think we should leave the tracing headers on by default. http://gerrit.cloudera.org:8080/#/c/19428/6/tests/shell/test_shell_commandline.py File tests/shell/test_shell_commandline.py: http://gerrit.cloudera.org:8080/#/c/19428/6/tests/shell/test_shell_commandline.py@29 PS6, Line 29: > Nit: already imported on line 42 removed this duplication -- To view, visit http://gerrit.cloudera.org:8080/19428 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7857eb5ec03eba32e06ec8d4133480f2e958ad2f Gerrit-Change-Number: 19428 Gerrit-PatchSet: 7 Gerrit-Owner: Jason Fehr <jf...@cloudera.com> Gerrit-Reviewer: Andrew Sherman <asher...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com> Gerrit-Comment-Date: Wed, 01 Feb 2023 16:48:39 +0000 Gerrit-HasComments: Yes
