David Knupp has posted comments on this change. ( http://gerrit.cloudera.org:8080/15378 )
Change subject: IMPALA-9466: impala-shell client retry for hs2-http protocol ...................................................................... Patch Set 3: (6 comments) First pass. I'll look at tests next, maybe later today or tomorrow. http://gerrit.cloudera.org:8080/#/c/15378/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/15378/3//COMMIT_MSG@18 PS3, Line 18: atleast Nit: at least http://gerrit.cloudera.org:8080/#/c/15378/3/shell/impala_client.py File shell/impala_client.py: http://gerrit.cloudera.org:8080/#/c/15378/3/shell/impala_client.py@645 PS3, Line 645: self.max_tries = 3 Design question -- is it better to a single max_tries value that belongs to the class? Would there ever be an argument for having a different number of retries for, say, ping rpcs vs open rpcs? Would it make sense to add a "max_tries" parameter to _do_hs2_rpc(), and have each type of operation define locally whether to retry, and if so, how many times? http://gerrit.cloudera.org:8080/#/c/15378/3/shell/impala_client.py@657 PS3, Line 657: r > I think in this case we need the lazy evaluation which lambda expressions p I'm not sure it makes a huge difference either way, but these do seem equivalent to me. >>> l = lambda: 1 >>> l.__name__ = 'return_one' >>> l <function return_one at 0x7f063d78b758> >>> >>> def return_one(): ... return 1 ... >>> d = return_one >>> d <function return_one at 0x7f063d78b7d0> >>> l() 1 >>> d() 1 The new named def can be nested inside the scope of the outer def, e.g. def _open_session(self): open_session_req = TOpenSessionReq(TProtocolVersion.HIVE_CLI_SERVICE_PROTOCOL_V6, username=self.user) def OpenSession(): return self.imp_service.OpenSession(open_session_req) rpc = OpenSession The only reason why I think it might matter is that an innocuous PEP-8 recommendation could easily become a requirement in future versions of python (which we've seen when trying to go from python 2 -> 3), so if this works (I didn't test it) it might be worth fixing it throughout. http://gerrit.cloudera.org:8080/#/c/15378/3/shell/impala_client.py@674 PS3, Line 674: raise_error = num_tries == max_tries Nit: minor style consideration. raise_error = (num_tries == max_tries) ...might be slightly more readable at a glance, since python allows chaining assignments like >>> foo = bar = True >>> foo True >>> bar True http://gerrit.cloudera.org:8080/#/c/15378/3/shell/impala_client.py@916 PS3, Line 916: raise_error = num_tries == max_tries Same nit as above: raise_error = (num_tries == max_tries) http://gerrit.cloudera.org:8080/#/c/15378/3/shell/impala_shell.py File shell/impala_shell.py: http://gerrit.cloudera.org:8080/#/c/15378/3/shell/impala_shell.py@1210 PS3, Line 1210: traceback.print_exc() Nice. Thanks for adding this. -- To view, visit http://gerrit.cloudera.org:8080/15378 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0da9e9e8d34a340eaf763397cc095ff6260d65d5 Gerrit-Change-Number: 15378 Gerrit-PatchSet: 3 Gerrit-Owner: Abhishek Rawat <ara...@cloudera.com> Gerrit-Reviewer: Abhishek Rawat <ara...@cloudera.com> Gerrit-Reviewer: David Knupp <dkn...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Sahil Takiar <stak...@cloudera.com> Gerrit-Comment-Date: Tue, 10 Mar 2020 17:54:02 +0000 Gerrit-HasComments: Yes
