Mailing-List: contact reviews-help@impala.incubator.apache.org; run by ezmlm
Precedence: bulk
Message-Id: <201710302136.v9ULaq13029633@ip-10-146-233-104.ec2.internal>
Date: Mon, 30 Oct 2017 21:36:52 +0000
From: "Alex Behm (Code Review)" <gerrit@cloudera.org>
To: Vuk Ercegovac <vercegovac@cloudera.com>, impala-cr@cloudera.com,
        reviews@impala.incubator.apache.org
CC: Dan Hecht <dhecht@cloudera.com>, Balazs Jeszenszky <jeszyb@gmail.com>,
        Philip Zeyliger <philip@cloudera.com>,
        Dimitris Tsirogiannis <dtsirogiannis@cloudera.com>,
        Michael Brown <mikeb@cloudera.com>
Subject: =?UTF-8?Q?=5BImpala-ASF-CR=5D_IMPALA-4704=3A_Turns_on_client_connections_when_local_catalog_initialized=2E=0A?=
In-Reply-To: <gerrit.1507075635000.I52b881cba18a7e4533e21a78751c2e35c3d4c8a6@gerrit.cloudera.org>
References: <gerrit.1507075635000.I52b881cba18a7e4533e21a78751c2e35c3d4c8a6@gerrit.cloudera.org>
Reply-To: alex.behm@cloudera.com, impala-cr@cloudera.com, marcelk@gmail.com,
        dhecht@cloudera.com, philip@cloudera.com,
        reviews@impala.incubator.apache.org, dtsirogiannis@cloudera.com,
        mikeb@cloudera.com, vercegovac@cloudera.com, jeszyb@gmail.com
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Content-Disposition: inline
User-Agent: Gerrit/2.14.2
Content-Type: multipart/alternative; boundary="MKrYOnB5wm4="; charset=UTF-8
archived-at: Mon, 30 Oct 2017 21:36:58 -0000

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

Alex Behm has posted comments on this change=2E ( http://gerrit=2Ecloudera=
=2Eorg:8080/8202 )

Change subject: IMPALA-4704: Turns on client connection=
s when local catalog initialized=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=2E


Patch Set 15:

(8 comments)

http://gerrit=2Ecloude=
ra=2Eorg:8080/#/c/8202/15/be/src/service/frontend=2Ecc
File be/src/service/=
frontend=2Ecc:

http://gerrit=2Ecloudera=2Eorg:8080/#/c/8202/15/be/src/serv=
ice/frontend=2Ecc@31
PS15, Line 31: #ifndef NDEBUG
What's the harm in alway=
s compiling it in?


http://gerrit=2Ecloudera=2Eorg:8080/#/c/8202/15/be/src=
/service/frontend=2Ecc@256
PS15, Line 256:   if (result) return true;
retur=
n result?


http://gerrit=2Ecloudera=2Eorg:8080/#/c/8202/15/be/src/service/=
impala-server=2Eh
File be/src/service/impala-server=2Eh:

http://gerrit=2Ec=
loudera=2Eorg:8080/#/c/8202/15/be/src/service/impala-server=2Eh@125
PS15, L=
ine 125:   /// Initializes client RPC services=2E Must be called after Init=
InternalServices=2E Is a
we typically say InitInternalServices() to make it=
 clear it's a function


http://gerrit=2Ecloudera=2Eorg:8080/#/c/8202/15/be=
/src/service/impala-server=2Ecc
File be/src/service/impala-server=2Ecc:

ht=
tp://gerrit=2Ecloudera=2Eorg:8080/#/c/8202/15/be/src/service/impala-server=
=2Ecc@1541
PS15, Line 1541:     // Additionally wait for the local catalog =
to be initialized if the server is a
Suggest rephrasing "wait" to clarify=
=2E "wait" could be misinterpreted as blocking=2E Maybe say something like:=


Announce the availability of this impalad coordinator once the local cata=
log has been initialized=2E


http://gerrit=2Ecloudera=2Eorg:8080/#/c/8202/=
15/be/src/service/impalad-main=2Ecc
File be/src/service/impalad-main=2Ecc:
=

http://gerrit=2Ecloudera=2Eorg:8080/#/c/8202/15/be/src/service/impalad-mai=
n=2Ecc@91
PS15, Line 91:   Status status =3D impala_server->StartInternalSe=
rvices();
As discussed, ordering is not quite right yet=2E

It might make s=
ense to add flags for client_services_started and internal_services_started=
 to address the race between opening internal/client ports and announcing t=
he availability of this daemon through the membership callback=2E


http://=
gerrit=2Ecloudera=2Eorg:8080/#/c/8202/15/bin/start-impala-cluster=2Epy
File=
 bin/start-impala-cluster=2Epy:

http://gerrit=2Ecloudera=2Eorg:8080/#/c/82=
02/15/bin/start-impala-cluster=2Epy@81
PS15, Line 81: # For testing: list o=
f comma-separated delays, in milliseconds, that delay catalog
clarify which=
 catalog exactly


http://gerrit=2Ecloudera=2Eorg:8080/#/c/8202/15/fe/src/m=
ain/java/org/apache/impala/service/Frontend=2Ejava
File fe/src/main/java/or=
g/apache/impala/service/Frontend=2Ejava:

http://gerrit=2Ecloudera=2Eorg:80=
80/#/c/8202/15/fe/src/main/java/org/apache/impala/service/Frontend=2Ejava@8=
77
PS15, Line 877:     }
add a final LOG=2Einfo() here declaring success (a=
nd remove the one in impala-server)


http://gerrit=2Ecloudera=2Eorg:8080/#=
/c/8202/12/fe/src/test/java/org/apache/impala/service/FrontendTest=2Ejava
F=
ile fe/src/test/java/org/apache/impala/service/FrontendTest=2Ejava:

http:/=
/gerrit=2Ecloudera=2Eorg:8080/#/c/8202/12/fe/src/test/java/org/apache/impal=
a/service/FrontendTest=2Ejava@a69
PS12, Line 69: 
> I was a bit surprised t=
o see authorization configs here-- what was the inte
Sentry can configured =
in a service mode or with a file-based auth policy=2E I suppose this test w=
as checking that the readiness check works regardless of auth policy=2E Not=
 sure how much sense that makes=2E

For auth we use AuthorizationTest=2E

I=
'm thinking this test doesn't make sense anymore=2E


-- 
To view, visit h=
ttp://gerrit=2Ecloudera=2Eorg:8080/8202
To unsubscribe, visit http://gerrit=
=2Ecloudera=2Eorg:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: =
master
Gerrit-MessageType: comment
Gerrit-Change-Id: I52b881cba18a7e4533e21=
a78751c2e35c3d4c8a6
Gerrit-Change-Number: 8202
Gerrit-PatchSet: 15
Gerrit-O=
wner: Vuk Ercegovac <vercegovac@cloudera=2Ecom>
Gerrit-Reviewer: Alex Behm =
<alex=2Ebehm@cloudera=2Ecom>
Gerrit-Reviewer: Balazs Jeszenszky <jeszyb@gma=
il=2Ecom>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera=2Ecom>
Gerrit-Reviewe=
r: Dimitris Tsirogiannis <dtsirogiannis@cloudera=2Ecom>
Gerrit-Reviewer: Mi=
chael Brown <mikeb@cloudera=2Ecom>
Gerrit-Reviewer: Philip Zeyliger <philip=
@cloudera=2Ecom>
Gerrit-Reviewer: Vuk Ercegovac <vercegovac@cloudera=2Ecom>=

Gerrit-Comment-Date: Mon, 30 Oct 2017 21:36:52 +0000
Gerrit-HasComments: Y=
es

--MKrYOnB5wm4=--