Mailing-List: contact reviews-help@impala.incubator.apache.org; run by ezmlm
Precedence: bulk
Message-Id: <201703242127.v2OLRKC1026027@ip-10-146-233-104.ec2.internal>
Date: Fri, 24 Mar 2017 21:27:19 +0000
From: "Dimitris Tsirogiannis (Code Review)" <gerrit@cloudera.org>
To: impala-cr@cloudera.com, reviews@impala.incubator.apache.org
CC: Henry Robinson <henry@cloudera.com>, Matthew Jacobs <mj@cloudera.com>,
        Dan Hecht <dhecht@cloudera.com>,
        Bharath Vissapragada <bharathv@cloudera.com>,
        Marcel Kornacker <marcel@cloudera.com>
Reply-To: dtsirogiannis@cloudera.com
Subject: =?UTF-8?Q?=5BImpala-ASF-CR=5D_IMPALA-4041=3A_Limit_catalog_and_admission_control_updates_to_coordinators=0A?=
In-Reply-To: <gerrit.1489173273000.I5f2c74abdbcd60ac050efa323616bd41182ceff3@gerrit.cloudera.org>
References: <gerrit.1489173273000.I5f2c74abdbcd60ac050efa323616bd41182ceff3@gerrit.cloudera.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Content-Disposition: inline
User-Agent: Gerrit/2.12.7
archived-at: Fri, 24 Mar 2017 21:27:25 -0000

Dimitris Tsirogiannis has posted comments on this change.

Change subject: IMPALA-4041: Limit catalog and admission control updates to coordinators
......................................................................


Patch Set 4:

(10 comments)

I am not so sure about removing the catalog and in-flight queries handles. They provide a way of visually validating that worker nodes do not receive catalog updates and that they don't schedule queries.

http://gerrit.cloudera.org:8080/#/c/6344/4/be/src/scheduling/scheduler.cc
File be/src/scheduling/scheduler.cc:

Line 259
> This line is effectively removed, right? It was added in: https://gerrit.cl
After inspecting the code and also talking to Lars, I believe this removal is safe. This change has the following effect. The scheduler has to wait for the statestore update that includes the local backend in order to add it to the backend config. Before that, the backend_config could be empty and scheduling should fail unless exec_at_coord is true. We have tests for these cases.


http://gerrit.cloudera.org:8080/#/c/6344/4/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:

Line 1588:     LOG(WARNING) << "Failed to convert hostname " << hostname << " to IP address";
> log status?
Done


PS4, Line 1601: address
> nit: descriptor
Done


PS4, Line 1604: }
              :   if (status.ok()) {
> else { 
Done


PS4, Line 1909: NULL
> nullptr
Done


PS4, Line 1931: NULL
> nullptr
Done


http://gerrit.cloudera.org:8080/#/c/6344/4/be/src/service/impala-server.h
File be/src/service/impala-server.h:

Line 436:   // it to the list of known backends and tell the statestore by adding it to
> this function isn't telling the statestore anything, it's only adding to to
Done


PS4, Line 438: In
> nit: should be To, not In
Done


http://gerrit.cloudera.org:8080/#/c/6344/4/bin/start-impala-cluster.py
File bin/start-impala-cluster.py:

PS4, Line 40: 3
> is there any way to make this default == the value of '-s'? If I run -s1 ri
Hm, not sure how to do that. I removed the check.


http://gerrit.cloudera.org:8080/#/c/6344/4/tests/custom_cluster/test_coordinators.py
File tests/custom_cluster/test_coordinators.py:

Line 83:       client2.close()
> can you add a check that the worker has not received the catalog metadata? 
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I5f2c74abdbcd60ac050efa323616bd41182ceff3
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dimitris Tsirogiannis <dtsirogiannis@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bharathv@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dtsirogiannis@cloudera.com>
Gerrit-Reviewer: Henry Robinson <henry@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <marcel@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj@cloudera.com>
Gerrit-HasComments: Yes