impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Michael Brown (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-5736: Add impala-shell argument to set default query options
Date Wed, 25 Oct 2017 18:22:58 GMT
Michael Brown has posted comments on this change. ( http://gerrit.cloudera.org:8080/8038 )

Change subject: IMPALA-5736: Add impala-shell argument to set default query options
......................................................................


Patch Set 15:

(6 comments)

I'm lukewarm on the exception hierarchy introduced. I appreciate the desire for clearer errors
for the user. However, in the case of exception hierarchies, PEP-008 [0] has this to say:

> Design exception hierarchies based on the distinctions that code catching the exceptions
is likely to need, rather than the locations where the exceptions are raised.

In the case of the exception hierarchy introduced in patch set 15, there isn't any catching
introduced: both exceptions are raised directly to the shell user.

Is it that important, then, to introduce this exception hierarchy at all?

Again however, I appreciate the desire to have clearer errors. At a high level, there are
three options I see:

1. Completely remove the exception hierarchy introduced.
2. Leave exception hierarchy in place, but trim declarations to be more conventional.
3. Leave the exception hierarchy in place, and leave its declarations in this more "verbose",
unconventional state.

I'm going to leave review comments guiding toward #2. Happy to hear arguments for 1 or 3.

[0] https://www.python.org/dev/peps/pep-0008/

http://gerrit.cloudera.org:8080/#/c/8038/15/shell/option_parser.py
File shell/option_parser.py:

http://gerrit.cloudera.org:8080/#/c/8038/15/shell/option_parser.py@36
PS15, Line 36: class OptionParserError(Exception):
             :   """Base class for option parser exceptions."""
             :   def __init__(self, msg):
             :     self.msg = msg
I think this should be removed. The only reason to have a hierarchy like this is if calling
code needs to except the whole group, i.e,

  except OptionParserError:

The calling code isn't catching any of these new exceptions.


http://gerrit.cloudera.org:8080/#/c/8038/15/shell/option_parser.py@41
PS15, Line 41: FileFormatError
ConfigFileFormatError might be a clearer name.


http://gerrit.cloudera.org:8080/#/c/8038/15/shell/option_parser.py@43
PS15, Line 43:   def __init__(self, msg):
             :     self.msg = msg
I don't you think you need to override __init__, because the Exception hierarchy already has
a builtin message attribute:

  >>> Exception.message
  <attribute 'message' of 'exceptions.BaseException' objects>
  >>>

That is, if you use the attribute "message" that already exists instead of rolling your own
"msg" attribute, you can make the declaration of this exception cleaner. However, I'm not
sure you need to do anything inside this class with "message" or "msg" if you follow the next
comment's advice.


http://gerrit.cloudera.org:8080/#/c/8038/15/shell/option_parser.py@46
PS15, Line 46:   def __str__(self):
             :     return "Unable to read configuration file correctly. Check formatting:
%s" % self.msg
Instead of overriding __str__, it is more conventional to let the message argument when raising
the exception be the full message of the exception. Let the message and clear exception class
name tell the full story. Taking this comment and the previous one together, this is the more
conventional way to create one's own exception:

  class MyError(ParentException):
    """docstring"""
    pass
  # ...
  raise MyError("full message of exception")


http://gerrit.cloudera.org:8080/#/c/8038/15/shell/option_parser.py@49
PS15, Line 49: InvalidValueError
InvalidOptionValueError?


http://gerrit.cloudera.org:8080/#/c/8038/15/shell/option_parser.py@51
PS15, Line 51:   def __init__(self, msg):
             :     self.msg = msg
             : 
             :   def __str__(self):
             :     return "Unexpected value in configuration file. %s" % self.msg
Same comments with __init__ and __str__ apply here.



-- 
To view, visit http://gerrit.cloudera.org:8080/8038
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I26a3b67230c80a99bd246b6af205d558fec9a986
Gerrit-Change-Number: 8038
Gerrit-PatchSet: 15
Gerrit-Owner: Csaba Ringhofer <csringhofer@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <csringhofer@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mjacobs@apache.org>
Gerrit-Reviewer: Michael Brown <mikeb@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <philip@cloudera.com>
Gerrit-Reviewer: anujphadke <aphadke@cloudera.com>
Gerrit-Comment-Date: Wed, 25 Oct 2017 18:22:58 +0000
Gerrit-HasComments: Yes

Mime
  • Unnamed multipart/alternative (inline, 8-Bit, 0 bytes)
View raw message