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 3: (11 comments) A few quick comments http://gerrit.cloudera.org:8080/#/c/19428/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/19428/3//COMMIT_MSG@2 PS3, Line 2: Author: jasonmfehr <jasonmf...@gmail.com> Do you want to use your Cloudera email? http://gerrit.cloudera.org:8080/#/c/19428/3//COMMIT_MSG@9 PS3, Line 9: When using the hs2 protocol with the http transport, include several tracing http The commit message should be wrapped at 732 characters. http://gerrit.cloudera.org:8080/#/c/19428/3/be/src/transport/THttpServer.h File be/src/transport/THttpServer.h: http://gerrit.cloudera.org:8080/#/c/19428/3/be/src/transport/THttpServer.h@156 PS3, Line 156: // Client-defined string identifying the HTTP request, meaingful only to the client. Nit "meaningful" http://gerrit.cloudera.org:8080/#/c/19428/3/be/src/transport/THttpServer.h@157 PS3, Line 157: const char* HEADER_REQUEST_ID = "X-Request-Id"; I think this should be something like: static const std::string HEADER_REQUEST_ID; and then, in THttpServer.cpp const string THttpTransport::HEADER_REQUEST_ID = "X-Request-Id"; http://gerrit.cloudera.org:8080/#/c/19428/3/be/src/transport/THttpServer.h@236 PS3, Line 236: std::string header_request_id_ = ""; To me its a bit confusing to have both HEADER_IMPALA_SESSION_ID and header_request_id_ Maybe x_request_id_ (etc.) is clearer? http://gerrit.cloudera.org:8080/#/c/19428/3/be/src/transport/THttpServer.cpp File be/src/transport/THttpServer.cpp: http://gerrit.cloudera.org:8080/#/c/19428/3/be/src/transport/THttpServer.cpp@245 PS3, Line 245: LOG(INFO) << "HTTP Connection Tracing" INFO means we we will usually log this, maybe this should be VLOG(2)? http://gerrit.cloudera.org:8080/#/c/19428/3/shell/impala_client.py File shell/impala_client.py: http://gerrit.cloudera.org:8080/#/c/19428/3/shell/impala_client.py@651 PS3, Line 651: # when the transport is http, subclasses can override this function Nit: capitalize sentence, add period at end. http://gerrit.cloudera.org:8080/#/c/19428/3/tests/shell/test_shell_commandline.py File tests/shell/test_shell_commandline.py: http://gerrit.cloudera.org:8080/#/c/19428/3/tests/shell/test_shell_commandline.py@1532 PS3, Line 1532: args = ['--protocol', 'hs2-http', '-q', 'select version();profile'] Add a brief description of the test http://gerrit.cloudera.org:8080/#/c/19428/3/tests/shell/test_shell_commandline.py@1554 PS3, Line 1554: # find all HTTP Connection Tracing log lines Capitalize comments and add periods at the end. http://gerrit.cloudera.org:8080/#/c/19428/3/tests/shell/test_shell_commandline.py@1560 PS3, Line 1560: # request_id consists of the same guid with a serially increasing integer Nit: make it clear that this is what impala-shell does. Other clients, or istio, may have different conventions. http://gerrit.cloudera.org:8080/#/c/19428/3/tests/shell/test_shell_commandline.py@1619 PS3, Line 1619: @CustomClusterTestSuite.with_args("-log_dir={0}".format(LOG_DIR_HTTP_TRACING)) You are running 2 tests with the same --log_dir value, can they interfere with each other? -- 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: 3 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-Comment-Date: Thu, 19 Jan 2023 03:46:46 +0000 Gerrit-HasComments: Yes
