Mailing-List: contact reviews-help@impala.incubator.apache.org; run by ezmlm
Precedence: bulk
Message-Id: <201703071835.v27IZ7PS017134@ip-10-146-233-104.ec2.internal>
Date: Tue, 7 Mar 2017 18:35:07 +0000
From: "Zach Amsden (Code Review)" <gerrit@cloudera.org>
To: impala-cr@cloudera.com, reviews@impala.incubator.apache.org
CC: Dimitris Tsirogiannis <dtsirogiannis@cloudera.com>,
        Dan Hecht <dhecht@cloudera.com>
Reply-To: zamsden@cloudera.com
Subject: =?UTF-8?Q?=5BImpala-ASF-CR=5D_IMPALA-4762=3A_RECOVER_PARTITIONS_should_batch_partition_updates=0A?=
In-Reply-To: <gerrit.1488844015000.I7f9334051b11ba8fa16159b7ca67ddc7f2392733@gerrit.cloudera.org>
References: <gerrit.1488844015000.I7f9334051b11ba8fa16159b7ca67ddc7f2392733@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: Tue, 07 Mar 2017 18:35:17 -0000

Zach Amsden has posted comments on this change.

Change subject: IMPALA-4762: RECOVER PARTITIONS should batch partition updates
......................................................................


Patch Set 2:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/6275/2/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java:

PS2, Line 2661: updateLastDdlTime(msTbl, msClient);
> Do we need to call this for every batch? I believe it is sufficient to call
If we get some kind of exception during a later round of processing, it is better to have updated the time, and this could reasonably happen with a timeout or network error.


http://gerrit.cloudera.org:8080/#/c/6275/2/tests/metadata/test_recover_partitions.py
File tests/metadata/test_recover_partitions.py:

PS2, Line 190: for i in xrange(1, 700):
             :         PART_DIR = "part%d\t" % i
             :         assert not self.has_value(PART_DIR, result.data)
> This check may be an overkill since no partitions were added to this table.
It does return one row, the 'Total', which is confusing as it isn't clear this is intended to be machine readable.  I'd rather leave the check as is since it is more obviously correct (and also fast, this is a purely local operation).


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I7f9334051b11ba8fa16159b7ca67ddc7f2392733
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Zach Amsden <zamsden@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dtsirogiannis@cloudera.com>
Gerrit-Reviewer: Zach Amsden <zamsden@cloudera.com>
Gerrit-HasComments: Yes