Mailing-List: contact reviews-help@impala.incubator.apache.org; run by ezmlm
Precedence: bulk
Message-Id: <201710251822.v9PIMwNR007447@ip-10-146-233-104.ec2.internal>
Date: Wed, 25 Oct 2017 18:22:58 +0000
From: "Michael Brown (Code Review)" <gerrit@cloudera.org>
To: Csaba Ringhofer <csringhofer@cloudera.com>, impala-cr@cloudera.com,
        reviews@impala.incubator.apache.org
CC: Matthew Jacobs <mjacobs@apache.org>, Lars Volker <lv@cloudera.com>,
        Philip Zeyliger <philip@cloudera.com>,
        anujphadke <aphadke@cloudera.com>
Subject: =?UTF-8?Q?=5BImpala-ASF-CR=5D_IMPALA-5736=3A_Add_impala-shell_argument_to_set_default_query_options=0A?=
In-Reply-To: <gerrit.1505231567000.I26a3b67230c80a99bd246b6af205d558fec9a986@gerrit.cloudera.org>
References: <gerrit.1505231567000.I26a3b67230c80a99bd246b6af205d558fec9a986@gerrit.cloudera.org>
Reply-To: mikeb@cloudera.com, impala-cr@cloudera.com, aphadke@cloudera.com,
        lv@cloudera.com, marcelk@gmail.com, csringhofer@cloudera.com,
        philip@cloudera.com, reviews@impala.incubator.apache.org,
        mjacobs@apache.org
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Content-Disposition: inline
User-Agent: Gerrit/2.14.2
Content-Type: multipart/alternative; boundary="aHcr9uEWRDs="; charset=UTF-8
archived-at: Wed, 25 Oct 2017 18:23:08 -0000

--aHcr9uEWRDs=
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable

Michael Brown has posted comments on this change=2E ( http://gerrit=2Ecloud=
era=2Eorg:8080/8038 )

Change subject: IMPALA-5736: Add impala-shell argume=
nt to set default query options
=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=
=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=
=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=
=2E=2E=2E=2E=2E=2E


Patch Set 15:

(6 comments)

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

> Design exception hierarchies based on the distinctions tha=
t code catching the exceptions is likely to need, rather than the locations=
 where the exceptions are raised=2E

In the case of the exception hierarchy=
 introduced in patch set 15, there isn't any catching introduced: both exce=
ptions are raised directly to the shell user=2E

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

Again however, I appreciat=
e the desire to have clearer errors=2E At a high level, there are three opt=
ions I see:

1=2E Completely remove the exception hierarchy introduced=2E
2=
=2E Leave exception hierarchy in place, but trim declarations to be more co=
nventional=2E
3=2E Leave the exception hierarchy in place, and leave its de=
clarations in this more "verbose", unconventional state=2E

I'm going to le=
ave review comments guiding toward #2=2E Happy to hear arguments for 1 or 3=
=2E

[0] https://www=2Epython=2Eorg/dev/peps/pep-0008/

http://gerrit=2Eclo=
udera=2Eorg:8080/#/c/8038/15/shell/option_parser=2Epy
File shell/option_par=
ser=2Epy:

http://gerrit=2Ecloudera=2Eorg:8080/#/c/8038/15/shell/option_par=
ser=2Epy@36
PS15, Line 36: class OptionParserError(Exception):
            =
 :   """Base class for option parser exceptions=2E"""
             :   def =
__init__(self, msg):
             :     self=2Emsg =3D msg
I think this sho=
uld be removed=2E The only reason to have a hierarchy like this is if calli=
ng code needs to except the whole group, i=2Ee,

  except OptionParserError=
:

The calling code isn't catching any of these new exceptions=2E


http://=
gerrit=2Ecloudera=2Eorg:8080/#/c/8038/15/shell/option_parser=2Epy@41
PS15, =
Line 41: FileFormatError
ConfigFileFormatError might be a clearer name=2E

=

http://gerrit=2Ecloudera=2Eorg:8080/#/c/8038/15/shell/option_parser=2Epy@4=
3
PS15, Line 43:   def __init__(self, msg):
             :     self=2Emsg =
=3D msg
I don't you think you need to override __init__, because the Except=
ion hierarchy already has a builtin message attribute:

  >>> Exception=2Em=
essage
  <attribute 'message' of 'exceptions=2EBaseException' 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 e=
xception cleaner=2E However, I'm not sure you need to do anything inside th=
is class with "message" or "msg" if you follow the next comment's advice=2E=


http://gerrit=2Ecloudera=2Eorg:8080/#/c/8038/15/shell/option_parser=2Epy=
@46
PS15, Line 46:   def __str__(self):
             :     return "Unable t=
o read configuration file correctly=2E Check formatting: %s" % self=2Emsg
I=
nstead of overriding __str__, it is more conventional to let the message ar=
gument when raising the exception be the full message of the exception=2E L=
et the message and clear exception class name tell the full story=2E 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
  # =2E=2E=2E
  raise MyError("full message of exce=
ption")


http://gerrit=2Ecloudera=2Eorg:8080/#/c/8038/15/shell/option_pars=
er=2Epy@49
PS15, Line 49: InvalidValueError
InvalidOptionValueError?


http=
://gerrit=2Ecloudera=2Eorg:8080/#/c/8038/15/shell/option_parser=2Epy@51
PS1=
5, Line 51:   def __init__(self, msg):
             :     self=2Emsg =3D ms=
g
             : 
             :   def __str__(self):
             :     re=
turn "Unexpected value in configuration file=2E %s" % self=2Emsg
Same comme=
nts with __init__ and __str__ apply here=2E


-- 
To view, visit http://ge=
rrit=2Ecloudera=2Eorg:8080/8038
To unsubscribe, visit http://gerrit=2Ecloud=
era=2Eorg:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
G=
errit-MessageType: comment
Gerrit-Change-Id: I26a3b67230c80a99bd246b6af205d=
558fec9a986
Gerrit-Change-Number: 8038
Gerrit-PatchSet: 15
Gerrit-Owner: Cs=
aba Ringhofer <csringhofer@cloudera=2Ecom>
Gerrit-Reviewer: Csaba Ringhofer=
 <csringhofer@cloudera=2Ecom>
Gerrit-Reviewer: Lars Volker <lv@cloudera=2Ec=
om>
Gerrit-Reviewer: Matthew Jacobs <mjacobs@apache=2Eorg>
Gerrit-Reviewer:=
 Michael Brown <mikeb@cloudera=2Ecom>
Gerrit-Reviewer: Philip Zeyliger <phi=
lip@cloudera=2Ecom>
Gerrit-Reviewer: anujphadke <aphadke@cloudera=2Ecom>
Ge=
rrit-Comment-Date: Wed, 25 Oct 2017 18:22:58 +0000
Gerrit-HasComments: Yes

--aHcr9uEWRDs=--