impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Bikramjeet Vig (Code Review)" <ger...@cloudera.org>
Subject [Impala-CR](cdh5-trunk) IMPALA-1683: Allow REFRESH on a single partition
Date Fri, 08 Jul 2016 22:07:37 GMT
Bikramjeet Vig has posted comments on this change.

Change subject: IMPALA-1683: Allow REFRESH on a single partition
......................................................................


Patch Set 5:

(12 comments)

http://gerrit.cloudera.org:8080/#/c/3385/5/fe/src/main/java/com/cloudera/impala/analysis/ResetMetadataStmt.java
File fe/src/main/java/com/cloudera/impala/analysis/ResetMetadataStmt.java:

PS5, Line 42: Preconditions.checkArgument(!isRefresh || name != null);
            :     Preconditions.checkArgument(partitionSpec == null || name != null);
> Can't we combine these two into a single check?
Done


http://gerrit.cloudera.org:8080/#/c/3385/5/fe/src/main/java/com/cloudera/impala/catalog/CatalogServiceCatalog.java
File fe/src/main/java/com/cloudera/impala/catalog/CatalogServiceCatalog.java:

PS5, Line 1164: To achieve that, it drops the partition
              :    * from the table object, fetches a fresh copy of the partition from Hive
              :    * metastore and adds it back to the table.
> We no longer do that in this function, so you may want to update the commen
Done


PS5, Line 1174: Db db = tbl.getDb();
> This seems to be used first time in L1195. Let's move it closer to that.
These 3 variables can be moved to L1195 but I kept them here because I wanted to minimize
amount of code inside critical sections(while holding locks)


PS5, Line 1175: HdfsPartition refreshedPartition = null;
> Where is this variable used?
Done


PS5, Line 1182: // For the case where partition does not exist in Catalog cache but might
              :       // exist in Hive Metastore
> I am not sure I follow the comment. I believe it's more accurate to say tha
Done


PS5, Line 1185: getPartitionNameFromThriftPartitionSpec(
> I think it's a bit more clear to call this function constructPartitionName(
Done


PS5, Line 1187: %s",
              :           tbl.getFullName() + " " + partitionName));
> "%s %s", tbl.getFullName(), partitionName);
Done


PS5, Line 1189: long newCatalogVersion = incrementAndGetCatalogVersion();
              :       catalogLock_.writeLock().unlock();
> Move this above L1180. We don't want to hold the catalog lock longer than n
Done


Line 1193:         org.apache.hadoop.hive.metastore.api.Partition partition = null;
> I would add a comment here saying: "If the partition does not exist in Hive
Done


PS5, Line 1206:     + tblName.getTable_name() + " " + partitionName,
              :               e);
> nit: merge these two lines
Done


http://gerrit.cloudera.org:8080/#/c/3385/5/fe/src/main/java/com/cloudera/impala/catalog/HdfsTable.java
File fe/src/main/java/com/cloudera/impala/catalog/HdfsTable.java:

PS5, Line 1944: data
> metadata (files and blocks)
Done


PS5, Line 1948: dropPartition(partitionSpec);
> We have a dropPartition function that takes as input a Partition object, so
the dropPartition function takes in a HdfsPartition type object but a "org.apache.hadoop.hive.metastore.api.Partition"
type object is being passed to this method. The only option was to use the dropPartition method
that took in the partitionSpec object. If I do not pass the partition spec object, I ll have
to write a helper method to create that object from the Partition object.

Also, the dropPartition method takes in the HdfsPartition that is already in catalog to remove
the stats and block data. If I pass it the new HdfsPartition object that I created, that will
mess up all the metadata.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4bf5dd630f2e175be5800187bbf9337c91f82bd8
Gerrit-PatchSet: 5
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Bikramjeet Vig <bikramjeet.vig@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bikramjeet.vig@cloudera.com>
Gerrit-Reviewer: David Knupp <dknupp@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dtsirogiannis@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message