impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Csaba Ringhofer (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-5736: Add impala-shell argument to set default query options
Date Mon, 30 Oct 2017 21:51:14 GMT
Csaba Ringhofer 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 16:

(6 comments)

> (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/

I am unsure about 1 vs 2. Probably no one will ever differentiate between these exceptions,
so they could be simply thrown as Exception. I have made the patch according to your comments
towards 2.

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 ConfigFileFormatError(Exception):
             :   """Raised when the config file cannot be read by ConfigParser."""
             :   pass
             : 
> I think this should be removed. The only reason to have a hierarchy like th
Done


http://gerrit.cloudera.org:8080/#/c/8038/15/shell/option_parser.py@41
PS15, Line 41: aised when an o
> ConfigFileFormatError might be a clearer name.
Done


http://gerrit.cloudera.org:8080/#/c/8038/15/shell/option_parser.py@43
PS15, Line 43: 
             : def parse_bool_opt
> I don't you think you need to override __init__, because the Exception hier
Done


http://gerrit.cloudera.org:8080/#/c/8038/15/shell/option_parser.py@46
PS15, Line 46:      Throws ValueError for other values.
             :   """
> Instead of overriding __str__, it is more conventional to let the message a
Done


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


http://gerrit.cloudera.org:8080/#/c/8038/15/shell/option_parser.py@51
PS15, Line 51:     return False
             :   else:
             :     raise InvalidOptionValueError("Unexpected value in configuration file.
'" + value \
             :       + "' is not a valid value for a boolean option.")
             : 
> Same comments with __init__ and __str__ apply here.
Done



-- 
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: 16
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: Mon, 30 Oct 2017 21:51:14 +0000
Gerrit-HasComments: Yes

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