Mailing-List: contact reviews-help@impala.incubator.apache.org; run by ezmlm
Precedence: bulk
Message-Id: <201710231842.v9NIg1ao011863@ip-10-146-233-104.ec2.internal>
Date: Mon, 23 Oct 2017 18:42:01 +0000
From: "Philip Zeyliger (Code Review)" <gerrit@cloudera.org>
To: impala-cr@cloudera.com, reviews@impala.incubator.apache.org
CC: Alex Behm <alex.behm@cloudera.com>, Michael Brown <mikeb@cloudera.com>,
        David Knupp <dknupp@cloudera.com>
Subject: =?UTF-8?Q?=5BImpala-ASF-CR=5D_IMPALA-6070=3A_Parallel_compute_table_stats=2Epy=0A?=
In-Reply-To: <gerrit.1508708043000.Ifb080f2552b9dbe304ecadd6e52429214094237d@gerrit.cloudera.org>
References: <gerrit.1508708043000.Ifb080f2552b9dbe304ecadd6e52429214094237d@gerrit.cloudera.org>
Reply-To: philip@cloudera.com, impala-cr@cloudera.com, dknupp@cloudera.com,
        marcelk@gmail.com, alex.behm@cloudera.com,
        reviews@impala.incubator.apache.org, mikeb@cloudera.com
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Content-Disposition: inline
User-Agent: Gerrit/2.14.2
Content-Type: multipart/alternative; boundary="bTZ4S5jCHpo="; charset=UTF-8
archived-at: Mon, 23 Oct 2017 18:42:06 -0000

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

Philip Zeyliger has posted comments on this change=2E ( http://gerrit=2Eclo=
udera=2Eorg:8080/8354 )

Change subject: IMPALA-6070: Parallel compute_tabl=
e_stats=2Epy
=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 3:

(4 comments)

http://gerrit=2Ecloudera=2Eorg:8080/#/c/8354=
/2/tests/util/compute_table_stats=2Epy
File tests/util/compute_table_stats=
=2Epy:

http://gerrit=2Ecloudera=2Eorg:8080/#/c/8354/2/tests/util/compute_t=
able_stats=2Epy@44
PS2, Line 44:       if not continue_on_error: raise e
> =
Two nits: even though we tend to do this a lot, PEP 8 frowns on inline if s=

Sure; fixed=2E

(This was pre-existing=2E)


http://gerrit=2Ecloudera=2Eor=
g:8080/#/c/8354/2/tests/util/compute_table_stats=2Epy@48
PS2, Line 48: 
> I=
'm curious -- why is it necessary to define the client factory inside of t
=
It's a closure=2E Doing it inline in the main block lets the closure access=
 options=2Eimpalad, and so on=2E Obviously, you could create a class, which=
 takes as init parameters all the options, and then make this a class metho=
d=2E Or you could use globals=2E

I've not changed anything here=2E


http:=
//gerrit=2Ecloudera=2Eorg:8080/#/c/8354/2/tests/util/compute_table_stats=2E=
py@71
PS2, Line 71: _factory, db, table, continue_on_error,)
> It seems to =
me like you don't need the intersection=2E You already have eithe
Let's say=
 all_tables =3D ["a", "b", "c"] and selected_tables =3D ["b", "c", "d"]=2E =
This code is trying to avoid running compute stats on "d", which has been s=
elected but isn't in all tables=2E


http://gerrit=2Ecloudera=2Eorg:8080/#/=
c/8354/2/tests/util/compute_table_stats=2Epy@116
PS2, Line 116:     yield i=
mpala_client
> Would it be possible to decorate client_factory as a @contex=
tmanager, somet
Done


-- 
To view, visit http://gerrit=2Ecloudera=2Eorg:8=
080/8354
To unsubscribe, visit http://gerrit=2Ecloudera=2Eorg:8080/settings=


Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comm=
ent
Gerrit-Change-Id: Ifb080f2552b9dbe304ecadd6e52429214094237d
Gerrit-Chan=
ge-Number: 8354
Gerrit-PatchSet: 3
Gerrit-Owner: Philip Zeyliger <philip@cl=
oudera=2Ecom>
Gerrit-Reviewer: Alex Behm <alex=2Ebehm@cloudera=2Ecom>
Gerri=
t-Reviewer: David Knupp <dknupp@cloudera=2Ecom>
Gerrit-Reviewer: Michael Br=
own <mikeb@cloudera=2Ecom>
Gerrit-Reviewer: Philip Zeyliger <philip@clouder=
a=2Ecom>
Gerrit-Comment-Date: Mon, 23 Oct 2017 18:42:01 +0000
Gerrit-HasCom=
ments: Yes

--bTZ4S5jCHpo=--