David Knupp has posted comments on this change. ( http://gerrit.cloudera.org:8080/15284 )
Change subject: IMPALA-9414 (part 2): Support the 'Expect: 100-continue' http header ...................................................................... Patch Set 6: (2 comments) http://gerrit.cloudera.org:8080/#/c/15284/6/shell/THttpClient.py File shell/THttpClient.py: http://gerrit.cloudera.org:8080/#/c/15284/6/shell/THttpClient.py@35 PS6, Line 35: class THttpClient(TTransportBase): I guess I'm not convinced that subclassing is better than straight copying, so this is fine. What do you think about the idea of renaming this to something like ImpalaThriftHttpClient, and adding a docstring or comment block at the top explaining the derivation and differences of this code. We have various places in our code (thirft_sasl.py and prettytable) where we copy a file or module from some other source repo, and it takes some digging to find out whether it's a straight copy or not, or what version is was copied from, and why. E.g., I don't know why we chose to copy thrift_sasl.py. And prettytable that shows up in our impala-python env as v0.7.2, even though it's not really the same as the upstream v0.7.2. Feel free to push back. http://gerrit.cloudera.org:8080/#/c/15284/1/shell/util.py File shell/util.py: http://gerrit.cloudera.org:8080/#/c/15284/1/shell/util.py@1 PS1, Line 1: > Ah okay. Seems like I could get it to work my adding a subdirectory, say sh I think your solution is fine. The main thing is just to put them all into one easy to find location. Having them all in impala_client.py always seemed kinda weird. -- To view, visit http://gerrit.cloudera.org:8080/15284 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4153968551acd58b25c7923c2ebf75ee29a7e76b Gerrit-Change-Number: 15284 Gerrit-PatchSet: 6 Gerrit-Owner: Thomas Tauber-Marshall <[email protected]> Gerrit-Reviewer: David Knupp <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Thomas Tauber-Marshall <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Thu, 05 Mar 2020 21:24:03 +0000 Gerrit-HasComments: Yes
