Jason Fehr has posted comments on this change. ( http://gerrit.cloudera.org:8080/19388 )
Change subject: IMPALA-11375 Impala shell outputs details of each RPC ...................................................................... Patch Set 9: (22 comments) comments addressed http://gerrit.cloudera.org:8080/#/c/19388/9//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/19388/9//COMMIT_MSG@2 PS9, Line 2: Author: jasonmfehr <jasonmf...@gmail.com> > If you want you can use a cloudera address Next change will use my cloudera email address. http://gerrit.cloudera.org:8080/#/c/19388/9//COMMIT_MSG@29 PS9, Line 29: already available in the standars output from the Impala shell. > Nit: "standard" Done http://gerrit.cloudera.org:8080/#/c/19388/9/shell/impala_client.py File shell/impala_client.py: http://gerrit.cloudera.org:8080/#/c/19388/9/shell/impala_client.py@701 PS9, Line 701: resp = self._do_hs2_rpc( > Nit: I think this style looks neater Done http://gerrit.cloudera.org:8080/#/c/19388/9/shell/impala_client.py@776 PS9, Line 776: # PingImpalaHS2Service rpc is idempotent and so safe to retry. > Nit: I expected a blank line before this That's actually the formatting used elsewhere, thus I did not change it. http://gerrit.cloudera.org:8080/#/c/19388/9/shell/impala_client.py@829 PS9, Line 829: self._clear_current_query_id() > This method seems unusual in that it doesn't use try...finally for the curr It was a miss on my part because this method is different. Adding the try...finally http://gerrit.cloudera.org:8080/#/c/19388/9/shell/impala_client.py@837 PS9, Line 837: self._current_query_id = self.get_query_id_str(query_handle) > We always calculate the _current_query_id even when show_rpc or rpc_file ar Done http://gerrit.cloudera.org:8080/#/c/19388/9/shell/impala_client.py@1216 PS9, Line 1216: f = open(self.rpc_file, "a") > It looks like we always open the file for append, so if I specify an existi The file is opened in append mode. That way, it behaves more like a log file. It's the safer option because existing data does not get deleted. If a user accidentally specifies the wrong output file, their data won't be erased. I added a sentence to the help text of the '--rpc_file' command line option explaining that it appends to existing files. http://gerrit.cloudera.org:8080/#/c/19388/9/shell/impala_client.py@1218 PS9, Line 1218: f.close() > Better maybe: Done http://gerrit.cloudera.org:8080/#/c/19388/9/shell/impala_client.py@1246 PS9, Line 1246: f = open(self.rpc_file, "a") > Use "with" as noted above? Done http://gerrit.cloudera.org:8080/#/c/19388/9/shell/option_parser.py File shell/option_parser.py: http://gerrit.cloudera.org:8080/#/c/19388/9/shell/option_parser.py@209 PS9, Line 209: parser.add_option("--show_rpc", dest="show_rpc", > Reading this I thought that I always needed to set show_rpc, and that I set I like this idea. Changing '--show_rpc' to '--rpc_stdout' and making changes to the help text to make it clearer. http://gerrit.cloudera.org:8080/#/c/19388/9/shell/option_parser.py@211 PS9, Line 211: help="Always show hs2 rpc details. Ignored if protocol is beeswax") > Nit: use 1 space? Done http://gerrit.cloudera.org:8080/#/c/19388/9/shell/option_parser.py@213 PS9, Line 213: help="If set, hs2 rpc call details are writte to the given file. " > Nit: "written" Done http://gerrit.cloudera.org:8080/#/c/19388/9/shell/thrift_printer.py File shell/thrift_printer.py: http://gerrit.cloudera.org:8080/#/c/19388/9/shell/thrift_printer.py@27 PS9, Line 27: to stdout, stderrr, or any other file handles.""" > Nit: "stderr" Done http://gerrit.cloudera.org:8080/#/c/19388/9/shell/thrift_printer.py@30 PS9, Line 30: # redacted_fields - list of names of object attributes who's > Nit: "whose" Done http://gerrit.cloudera.org:8080/#/c/19388/9/shell/thrift_printer.py@33 PS9, Line 33: # base indentation where all other indetations > Nit: "indentations" Done http://gerrit.cloudera.org:8080/#/c/19388/9/shell/thrift_printer.py@36 PS9, Line 36: # will not be be printed out, useful to ignore > Nit: "will not be" Done http://gerrit.cloudera.org:8080/#/c/19388/9/shell/thrift_printer.py@56 PS9, Line 56: be anything that has a write(string) method on it. > Nit: just "has a write(string) method" may be clearer? Done http://gerrit.cloudera.org:8080/#/c/19388/9/shell/thrift_printer.py@64 PS9, Line 64: # thrfit_obj - the object to print out, its attributes will > Nit: "thrift_obj" Done http://gerrit.cloudera.org:8080/#/c/19388/9/shell/thrift_printer.py@69 PS9, Line 69: # write(str) method on it > Nit: just "has a write(string) method" may be clearer? Done http://gerrit.cloudera.org:8080/#/c/19388/9/shell/thrift_printer.py@96 PS9, Line 96: # TODO -- print out attributes in alphabetical order > Looks like this is done already? yes, removed comment http://gerrit.cloudera.org:8080/#/c/19388/9/tests/shell/test_shell_commandline.py File tests/shell/test_shell_commandline.py: http://gerrit.cloudera.org:8080/#/c/19388/9/tests/shell/test_shell_commandline.py@1412 PS9, Line 1412: def test_output_rpc_to_screen(self, vector, populated_table, tmp_file): > Add a brief description of what the test does. Done http://gerrit.cloudera.org:8080/#/c/19388/9/tests/shell/test_shell_commandline.py@1422 PS9, Line 1422: def check_multiline(assert_str, regex_lines): > Add one line description of function Done -- To view, visit http://gerrit.cloudera.org:8080/19388 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I36f8dbc96726aa2a573133acbe8a558299381f8b Gerrit-Change-Number: 19388 Gerrit-PatchSet: 9 Gerrit-Owner: Jason Fehr <jf...@cloudera.com> Gerrit-Reviewer: Abhishek Rawat <ara...@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: Thu, 05 Jan 2023 17:52:52 +0000 Gerrit-HasComments: Yes
