impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alex Behm (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-4033,IMPALA-4105: Improvements of partition DDL.
Date Fri, 18 Nov 2016 22:20:19 GMT
Alex Behm has posted comments on this change.

Change subject: IMPALA-4033,IMPALA-4105: Improvements of partition DDL.
......................................................................


Patch Set 1:

(8 comments)

Thanks for continuing this work! Can you please separate this patch into two ones, one for
each JIRA? That generally makes it easier for us to do backports of bugfixes.

http://gerrit.cloudera.org:8080/#/c/5137/1/bin/start-catalogd.sh
File bin/start-catalogd.sh:

Line 54:   export JAVA_TOOL_OPTIONS="-agentlib:jdwp=transport=dt_socket,address=${JVM_DEBUG_PORT},server=y,suspend=${JVM_SUSPEND}
${JAVA_TOOL_OPTIONS}"
what's the motivation for this change?


http://gerrit.cloudera.org:8080/#/c/5137/1/common/thrift/CatalogService.thrift
File common/thrift/CatalogService.thrift:

Line 192: 
replace "some" with "the"


http://gerrit.cloudera.org:8080/#/c/5137/1/fe/src/main/java/org/apache/impala/analysis/PartitionSet.java
File fe/src/main/java/org/apache/impala/analysis/PartitionSet.java:

Line 163
this fix needs a test


http://gerrit.cloudera.org:8080/#/c/5137/1/fe/src/main/java/org/apache/impala/analysis/PartitionSpec.java
File fe/src/main/java/org/apache/impala/analysis/PartitionSpec.java:

Line 73
The toLowerCase() in this file seem correct to me, why did you remove them?


http://gerrit.cloudera.org:8080/#/c/5137/1/fe/src/main/java/org/apache/impala/analysis/ResetMetadataStmt.java
File fe/src/main/java/org/apache/impala/analysis/ResetMetadataStmt.java:

Line 40:   // not null when refreshing some partition(s)
replace "some" with "a set of"


Line 47:     this.tableName_ = name;
can remove 'this' everywhere in this constructor


Line 74:           TableRef tableRef = new TableRef(tableName_.toPath(), null, Privilege.ALTER);
This is a pretty drastic behavioral change. Look at the comment on L62 following. We basically
want to avoid loading the entire table metadata for such a refresh. However, we need the table
metadata to fetch the partitions corresponding to the predicates. It's not really clear to
me what we should do here, but regressing the existing behavior seems very dangerous.

I'll need some time to think about this. Feel free to propose ideas :)


http://gerrit.cloudera.org:8080/#/c/5137/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
File fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java:

Line 1256:             hmsPartition = msClient.getHiveClient().getPartition(
use the bulk getPartitions() API to get all partitions in one RPC


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I0f4e46ec0a63b46e485141290268d019c3dd15c7
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Amos Bird <amosbird@gmail.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message