impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dimitris Tsirogiannis (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-4524: Batch calls to ALTER TABLE...ADD PARTITION.
Date Thu, 12 Oct 2017 16:59:32 GMT
Dimitris Tsirogiannis has posted comments on this change. ( http://gerrit.cloudera.org:8080/8238
)

Change subject: IMPALA-4524: Batch calls to ALTER TABLE...ADD PARTITION.
......................................................................


Patch Set 3:

(2 comments)

Re testing, a simple python test that adds at least MAX_PARTITION_UPDATES_PER_SEC + 1 partitions
and then validates that they were actually added in the HMS (e.g. by doing show partitions)
is sufficient. If you want you can convert the analysis test in AnalyzeDDLTest.java to a positive
test but it is kind of redundant if you add the python test I just mentioned.

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

http://gerrit.cloudera.org:8080/#/c/8238/3/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@a1954
PS3, Line 1954: 
Why did you remove this?


http://gerrit.cloudera.org:8080/#/c/8238/3/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1974
PS3, Line 1974: if (hmsSublist.size() != addedHmsPartitions.size()) {
              :         List<Partition> difference = computeDifference(hmsSublist,
              :             addedHmsPartitions);
              :         addedHmsPartitions.addAll(
              :             getPartitionsFromHms(msTbl, msClient, tableName, difference));
              :       }
              : 
              :       for (Partition partition: addedHmsPartitions) {
              :         // Create and add the HdfsPartition to catalog. Return the table object
with an
              :         // updated catalog version.
              :         addHdfsPartition(tbl, partition);
              :       }
I think you can refactor this so that you make only one call to getPartitionsFromHms() outside
the for loop.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I95f8221ff08c0f126f951f7d37ff5e57985f855f
Gerrit-Change-Number: 8238
Gerrit-PatchSet: 3
Gerrit-Owner: Philip Zeyliger <philip@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dtsirogiannis@cloudera.com>
Gerrit-Comment-Date: Thu, 12 Oct 2017 16:59:32 +0000
Gerrit-HasComments: Yes

Mime
  • Unnamed multipart/alternative (inline, 8-Bit, 0 bytes)
View raw message