Bharath Krishna has posted comments on this change. ( http://gerrit.cloudera.org:8080/13049 )
Change subject: IMPALA-8149 : Add support for alter_database events ...................................................................... Patch Set 4: (4 comments) Overall looks good, leaving a few comments.. http://gerrit.cloudera.org:8080/#/c/13049/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/13049/4/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@757 PS4, Line 757: * If tblName is null, applies to database. We can say: if tblName is null, removes version number from database. if tblName is not null and Table is not incomplete, removes version number from Table. http://gerrit.cloudera.org:8080/#/c/13049/1/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/13049/1/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3805 PS1, Line 3805: if (db == null) { > Remove it should be fine? I think removing is fine. Probably we can change the Preconditions.checkState() in catalog_.getDb to PreConditions.checkArguments() as Fredy suggested? http://gerrit.cloudera.org:8080/#/c/13049/4/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/13049/4/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@345 PS4, Line 345: I feel we should add an additional test, here we are testing if the Notification Processor is successfully processing the notification events generated from Hive. We are not triggering the AlterDatabase operation from Impala in this test. (We need to do something similar to MetastoreEventsProcessorTest#alterTableAddColsFromImpala). http://gerrit.cloudera.org:8080/#/c/13049/4/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@1481 PS4, Line 1481: eventsProcessor_.processEvents(); Can we check if possible that the counter MetastoreEventsProcessor.EVENTS_SKIPPED_METRIC is updated correctly here, as we are skipping some events? -- To view, visit http://gerrit.cloudera.org:8080/13049 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iaf020e85cae04163bf32e31363eb4119d624640b Gerrit-Change-Number: 13049 Gerrit-PatchSet: 4 Gerrit-Owner: Anonymous Coward <xiaom...@cloudera.com> Gerrit-Reviewer: Anonymous Coward <xiaom...@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: Thu, 25 Apr 2019 20:46:33 +0000 Gerrit-HasComments: Yes
