Andrew Sherman 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 6: (2 comments) More comments, I think these are the last 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@250 PS6, Line 250: << (header_x_request_id_.empty() ? "" : " request_id=" + header_x_request_id_) I think the log output should also be x-request-id (or similar). This makes it clear to any reader what it means. Also, in the case of session_id, it is clearer that this is what the client tells us the session id is. It could be a lie! Or more perhaps also unlikely, but possible, a bug. But by using x-xxx we are clear that this is something from the client, so the log reader knows what it is they are seeing. 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_tracing"? It's clearer in the code, it's clearer to have a positive option (so we don't have a no_xxx option). -- 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: 6 Gerrit-Owner: Jason Fehr <[email protected]> Gerrit-Reviewer: Andrew Sherman <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Jason Fehr <[email protected]> Gerrit-Comment-Date: Mon, 23 Jan 2023 03:39:32 +0000 Gerrit-HasComments: Yes
