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 3: (9 comments) addressed all 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 <[email protected]> > Do you want to use your Cloudera email? Done 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. Done 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" Done 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: I initially used a static const std::string, but these constants are used in calls to THRIFT_strncasecmp which takes a char* as input. I would prefer to use a std::string but could not find an easy way to pass it to a function that needs a char*. If there is an easy way I missed, please let me know. 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_ Done 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. Done 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 Done 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. Done 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. Done -- 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 <[email protected]> Gerrit-Reviewer: Andrew Sherman <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Jason Fehr <[email protected]> Gerrit-Comment-Date: Fri, 20 Jan 2023 15:40:33 +0000 Gerrit-HasComments: Yes
