Anurag Mantripragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/15648 )
Change subject: IMPALA-8632: Add support for self-event detection for insert events ...................................................................... Patch Set 4: (15 comments) Thanks for the change, I have added some comments, most of them are style-related and some nits. http://gerrit.cloudera.org:8080/#/c/15648/4/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/15648/4/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@54 PS4, Line 54: import org.apache.impala.catalog.events.NoOpEventProcessor; Nit: Duplicate import, could you remove this? http://gerrit.cloudera.org:8080/#/c/15648/4/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@934 PS4, Line 934: Nit: Incorrect indentation. Remove the empty spaces. http://gerrit.cloudera.org:8080/#/c/15648/4/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@937 PS4, Line 937: Incorrect indentation. Remove the empty spaces. http://gerrit.cloudera.org:8080/#/c/15648/4/fe/src/main/java/org/apache/impala/catalog/Table.java File fe/src/main/java/org/apache/impala/catalog/Table.java: http://gerrit.cloudera.org:8080/#/c/15648/4/fe/src/main/java/org/apache/impala/catalog/Table.java@807 PS4, Line 807: Nit: Incorrect indentation. Remove the empty spaces. http://gerrit.cloudera.org:8080/#/c/15648/4/fe/src/main/java/org/apache/impala/catalog/Table.java@826 PS4, Line 826: Nit: Incorrect indentation. Remove the empty spaces. http://gerrit.cloudera.org:8080/#/c/15648/4/fe/src/main/java/org/apache/impala/catalog/events/InFlightEvents.java File fe/src/main/java/org/apache/impala/catalog/events/InFlightEvents.java: http://gerrit.cloudera.org:8080/#/c/15648/4/fe/src/main/java/org/apache/impala/catalog/events/InFlightEvents.java@65 PS4, Line 65: eventIds_ Here and everywhere else, if this means "only" insert events, may I suggest using insertEventIds_? http://gerrit.cloudera.org:8080/#/c/15648/4/fe/src/main/java/org/apache/impala/catalog/events/InFlightEvents.java@69 PS4, Line 69: @param isInsertEvent if true, return list of versions for in-flight Insert events : * if false, return list of eventIds for in-flight DDL events Should this be the other way around? i.e., return eventIds for in-flight insert events if it is insert events and versions if it is not. Also, indentation is incorrect, please remove the extra spaces. http://gerrit.cloudera.org:8080/#/c/15648/4/fe/src/main/java/org/apache/impala/catalog/events/InFlightEvents.java@87 PS4, Line 87: Nit: Indentation incorrect, remove extra spaces. http://gerrit.cloudera.org:8080/#/c/15648/4/fe/src/main/java/org/apache/impala/catalog/events/InFlightEvents.java@107 PS4, Line 107: Nit: Indentation incorrect, remove extra spaces. http://gerrit.cloudera.org:8080/#/c/15648/4/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/15648/4/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@773 PS4, Line 773: // long eventId_test = catalog_.public_eventId.consumeId(); Could you remove this comment? http://gerrit.cloudera.org:8080/#/c/15648/4/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@787 PS4, Line 787: /** : * Check self-event for in flight Insert event using eventId : */ Nit: Not sure this is needed since this is an overridden method and its purpose is the same as described in L318. http://gerrit.cloudera.org:8080/#/c/15648/4/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/15648/4/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@75 PS4, Line 75: *; We should be careful with *. Are you sure we are importing everything from org.apache.impala.catalog? http://gerrit.cloudera.org:8080/#/c/15648/4/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@4424 PS4, Line 4424: List of all catalog object that was insert into Did you mean "List of all partitions we insert into"? http://gerrit.cloudera.org:8080/#/c/15648/4/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@4487 PS4, Line 4487: // Firing insert events by making calls to HMS APIs can be slow for tables with : // large number of partitions. > this comment should be updated with the bulk API it should not be a problem I see that MetastoreShim.fireInsertEvent() for hive-2 does not insert in bulk and is still making one RPC per partition. We should keep the earlier asynchronous fireInsertEvent() for hive-2. http://gerrit.cloudera.org:8080/#/c/15648/4/fe/src/main/java/org/apache/impala/util/MetaStoreUtil.java File fe/src/main/java/org/apache/impala/util/MetaStoreUtil.java: http://gerrit.cloudera.org:8080/#/c/15648/4/fe/src/main/java/org/apache/impala/util/MetaStoreUtil.java@33 PS4, Line 33: import org.apache.hadoop.hive.metastore.api.FireEventRequest; : import org.apache.hadoop.hive.metastore.api.FireEventRequestData; : import org.apache.hadoop.hive.metastore.api.FireEventResponse; : import org.apache.hadoop.hive.metastore.api.InsertEventRequestData; These imports are not needed anymore. -- To view, visit http://gerrit.cloudera.org:8080/15648 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7873fbb2c159343690f93b9d120f6b425b983dcf Gerrit-Change-Number: 15648 Gerrit-PatchSet: 4 Gerrit-Owner: Xiaomeng Zhang <xiaom...@cloudera.com> Gerrit-Reviewer: Anurag Mantripragada <anu...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Vihang Karajgaonkar <vih...@cloudera.com> Gerrit-Reviewer: Xiaomeng Zhang <xiaom...@cloudera.com> Gerrit-Comment-Date: Tue, 07 Apr 2020 23:47:36 +0000 Gerrit-HasComments: Yes
