Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/17308 )
Change subject: IMPALA-10502: Handle CREATE/DROP events correctly ...................................................................... Patch Set 12: Code-Review+1 (6 comments) Carrying my +1. Looked at the changes between PS7 and PS12, I'm good with the test coverage. Thanks for this work! http://gerrit.cloudera.org:8080/#/c/17308/12/fe/src/main/java/org/apache/impala/catalog/events/NoOpEventProcessor.java File fe/src/main/java/org/apache/impala/catalog/events/NoOpEventProcessor.java: http://gerrit.cloudera.org:8080/#/c/17308/12/fe/src/main/java/org/apache/impala/catalog/events/NoOpEventProcessor.java@20 PS12, Line 20: import org.apache.hadoop.hive.metastore.api.NotificationEvent; : import org.apache.impala.catalog.CatalogException; : import org.apache.impala.catalog.events.MetastoreEvents.IgnoredEvent; : import org.apache.impala.catalog.events.MetastoreEvents.MetastoreEvent; nit: seems unused imports http://gerrit.cloudera.org:8080/#/c/17308/12/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/17308/12/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@693 PS12, Line 693: debug Do we change the log level somewhere? I see these debug loggings when running test_event_processor.py. http://gerrit.cloudera.org:8080/#/c/17308/12/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@804 PS12, Line 804: nit: don't need space http://gerrit.cloudera.org:8080/#/c/17308/12/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@805 PS12, Line 805: nit: don't need space http://gerrit.cloudera.org:8080/#/c/17308/12/tests/custom_cluster/test_event_processing.py File tests/custom_cluster/test_event_processing.py: http://gerrit.cloudera.org:8080/#/c/17308/12/tests/custom_cluster/test_event_processing.py@666 PS12, Line 666: "insert into table {0}.{1} partition (year,month) " nit: Could you also add an OVERWRITE insert case? E.g. split this into insert into table {0}.{1} partition(year, month) select * from functional.alltypessmall where month % 2 = 0 insert overwrite table {0}.{1} partition(year, month) select * from functional.alltypessmall where month % 2 = 1 Just thinking about covering more cases. http://gerrit.cloudera.org:8080/#/c/17308/12/tests/custom_cluster/test_event_processing.py@709 PS12, Line 709: # the table is empty so the following inserts add 0 rows nit: seems a wrong comment since "functional.alltypestiny" is not empty.. -- To view, visit http://gerrit.cloudera.org:8080/17308 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia2c5e96b48abac015240f20295b3ec3b1d71f24a Gerrit-Change-Number: 17308 Gerrit-PatchSet: 12 Gerrit-Owner: Vihang Karajgaonkar <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Sourabh Goyal <[email protected]> Gerrit-Reviewer: Vihang Karajgaonkar <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Fri, 25 Jun 2021 12:44:07 +0000 Gerrit-HasComments: Yes
