David Knupp has posted comments on this change. Change subject: IMPALA-5333: Add support for Impala to work with ADLS ......................................................................
Patch Set 2: (4 comments) http://gerrit.cloudera.org:8080/#/c/6910/2/tests/util/adls_util.py File tests/util/adls_util.py: Line 29: @classmethod This seems really unusual to me. __init__ methods typically don't need a @classmethod decorator, since what you're initializing is an instance of the class, not the class itself. By adding the decorator, "self" here becomes a reference to the class, not the instance. Perhaps there's something more exotic going on here that I'm not picking up on though. Let me know if I'm off target with this. I've just never seen this done. PS2, Line 40: f.close() > I believe this is redundant but I could be wrong. You're correct -- "with ... as foo" should close foo for you. Line 45: self.adlsclient.mkdir(path) What is the return value of self.adlsclient.mkdir(path)? Does it make sense to return it instead? What happens if the call fails? Line 72: files = self.ls(path) Super insignificant nit: you could put this inside of the list comprehension to make this method a tidy one-liner. -- To view, visit http://gerrit.cloudera.org:8080/6910 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic56b9988b32a330443f24c44f9cb2c80842f7542 Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Sailesh Mukil <sail...@cloudera.com> Gerrit-Reviewer: Attila Jeges <atti...@cloudera.com> Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com> Gerrit-Reviewer: David Knupp <dkn...@cloudera.com> Gerrit-Reviewer: Dimitris Tsirogiannis <dtsirogian...@cloudera.com> Gerrit-Reviewer: Matthew Jacobs <m...@cloudera.com> Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com> Gerrit-HasComments: Yes
