Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/13111 )
Change subject: IMPALA-7973: Add support for fine grained events processing for partition level HMS events. ...................................................................... Patch Set 6: (8 comments) http://gerrit.cloudera.org:8080/#/c/13111/6/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java File fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java: http://gerrit.cloudera.org:8080/#/c/13111/6/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@2113 PS6, Line 2113: Throws CatalogException if partition reload is unsuccessful. Throws : * DatabaseNotFoundException if Db doesn't exist We should use @throws CatalogException and @throws DatabaseNotFoundException javadoc instead. http://gerrit.cloudera.org:8080/#/c/13111/6/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java File fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java: http://gerrit.cloudera.org:8080/#/c/13111/6/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@1252 PS6, Line 1252: /** nit: add a new line after L1251 http://gerrit.cloudera.org:8080/#/c/13111/6/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@1255 PS6, Line 1255: getTPartSpecFromHmsPart I think it's better to not shorten partition as part since it can be confusing. http://gerrit.cloudera.org:8080/#/c/13111/6/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@1272 PS6, Line 1272: constructPartStringFromTpart nit: constructPartitionStringFromTPartition http://gerrit.cloudera.org:8080/#/c/13111/6/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@1288 PS6, Line 1288: List<Partition> addedPartitions_; can this be private final? http://gerrit.cloudera.org:8080/#/c/13111/6/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@1463 PS6, Line 1463: private final List<Map<String, String>> droppedPartitions_; nit: add a new empty line after this variable declaration http://gerrit.cloudera.org:8080/#/c/13111/6/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@1523 PS6, Line 1523: a typo: an http://gerrit.cloudera.org:8080/#/c/13111/6/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java File fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java: http://gerrit.cloudera.org:8080/#/c/13111/6/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@1573 PS6, Line 1573: Collection<? extends FeFsPartition> partsAfterAdd = We don't have to do it in this CR, but the method here is getting too big. We should have a separate helper method for each event. It' makes the code easier to read. -- To view, visit http://gerrit.cloudera.org:8080/13111 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I213401329f3965dd81055197792ccf8a05368af5 Gerrit-Change-Number: 13111 Gerrit-PatchSet: 6 Gerrit-Owner: Anurag Mantripragada <anu...@cloudera.com> Gerrit-Reviewer: Anurag Mantripragada <anu...@cloudera.com> Gerrit-Reviewer: Bharath Krishna <bhar...@cloudera.com> Gerrit-Reviewer: Bharath Vissapragada <bhara...@cloudera.com> Gerrit-Reviewer: Fredy Wijaya <fwij...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Vihang Karajgaonkar <vih...@cloudera.com> Gerrit-Comment-Date: Tue, 30 Apr 2019 00:54:18 +0000 Gerrit-HasComments: Yes
