Sahil Takiar has posted comments on this change. ( http://gerrit.cloudera.org:8080/16093 )
Change subject: IMPALA-9213: Add query retry info to GetLog result ...................................................................... Patch Set 6: (5 comments) http://gerrit.cloudera.org:8080/#/c/16093/6/be/src/service/impala-beeswax-server.cc File be/src/service/impala-beeswax-server.cc: http://gerrit.cloudera.org:8080/#/c/16093/6/be/src/service/impala-beeswax-server.cc@327 PS6, Line 327: error_log_ss << Substitute(GET_LOG_QUERY_RETRY_INFO_FORMAT, might be worth adding a DCHECK right before this line asserting that '!original_query_handle->query_status()' http://gerrit.cloudera.org:8080/#/c/16093/6/be/src/service/impala-hs2-server.cc File be/src/service/impala-hs2-server.cc: http://gerrit.cloudera.org:8080/#/c/16093/6/be/src/service/impala-hs2-server.cc@939 PS6, Line 939: ss << Substitute(GET_LOG_QUERY_RETRY_INFO_FORMAT, same comment as the beeswax version http://gerrit.cloudera.org:8080/#/c/16093/6/be/src/service/impala-server.cc File be/src/service/impala-server.cc: http://gerrit.cloudera.org:8080/#/c/16093/6/be/src/service/impala-server.cc@366 PS6, Line 366: \n don't think this is necessary? it looks like it is adding an extra new line in the error output: WARNINGS: Original query failed: Failed due to unreachable impalad(s): stakiar-desktop:22002 Query has been retried using query id: c84bd698dc98fac2:e79c78d100000000 Unless that was intentional? It looks like Status::GetDetail --> ErrorMsg::GetFullMessageDetails which always has a newline at the end of the stringstream http://gerrit.cloudera.org:8080/#/c/16093/6/shell/impala_client.py File shell/impala_client.py: http://gerrit.cloudera.org:8080/#/c/16093/6/shell/impala_client.py@337 PS6, Line 337: get_query_link_callback instead of piping this through all these methods as a callback, can we just move the 'get_query_link' method into a util class and just import it and use it directly here and in impala_shell.py? you might be able to just add it as a utility method in impala_client.py and import it in impala_shell.py. I guess there isn't a common util class. http://gerrit.cloudera.org:8080/#/c/16093/6/shell/impala_shell.py File shell/impala_shell.py: http://gerrit.cloudera.org:8080/#/c/16093/6/shell/impala_shell.py@1072 PS6, Line 1072: _get_query_link_callback nit: add some method code comments, rename to '_get_query_link' since it isn't always used as a callback (e.g. in _periodic_wait_callback) -- To view, visit http://gerrit.cloudera.org:8080/16093 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I58cf94f91a0b92eb9a3088bee3894ac157a954dc Gerrit-Change-Number: 16093 Gerrit-PatchSet: 6 Gerrit-Owner: Quanlong Huang <huangquanl...@gmail.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Quanlong Huang <huangquanl...@gmail.com> Gerrit-Reviewer: Sahil Takiar <stak...@cloudera.com> Gerrit-Comment-Date: Fri, 26 Jun 2020 22:51:37 +0000 Gerrit-HasComments: Yes
