impala-reviews mailing list archives

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

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


Patch Set 1:

(8 comments)

Ah, sorry. I saw there are other commits targeting more than one issues. What should I do
now? abandon this?

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?
It's for remote debuging catalog's jvm. This is like what start-impalad.sh does.


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

Line 192: 
> replace "some" with "the"
Done


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
Done


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?
Done


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"
Done


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


Line 74:           TableRef tableRef = new TableRef(tableName_.toPath(), null, Privilege.ALTER);
> This is a pretty drastic behavioral change. Look at the comment on L62 foll
yeah, it's really a dillema here. My rough idea is to loop in the old partitionSpec as a special
case.


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
Done


-- 
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-Reviewer: Amos Bird <amosbird@gmail.com>
Gerrit-HasComments: Yes

Mime
View raw message