xiaom...@cloudera.com has posted comments on this change. ( http://gerrit.cloudera.org:8080/13049 )
Change subject: IMPALA-8149 : Add support for alter_database events ...................................................................... Patch Set 5: (8 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, removes version number from database. > We can say: Done http://gerrit.cloudera.org:8080/#/c/13049/1/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/13049/1/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@445 PS1, Line 445: otected static Stri > We can just leave it for now. Removing the check is wrong if params can be Done 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) { > I think removing is fine. Probably we can change the Preconditions.checkSta Done http://gerrit.cloudera.org:8080/#/c/13049/1/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3812 PS1, Line 3812: default: > Moved it under set_owner which lock db first. Done http://gerrit.cloudera.org:8080/#/c/13049/1/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3813 PS1, Line 3813: throw new UnsupportedOperationException( > I copied the format in alterTable method which update catalogServiceIdentif Done 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@343 PS4, Line 343: est > nit: fix indentation I was using clang tool do formatting, I guess it takes each argument as indent 4, the inside argument break indented 4 more spaces on original. >From Google: "two continuation lines use the same indentation level if and >only if they begin with syntactically parallel elements." So I think it make sense to indent this way. What do you think? http://gerrit.cloudera.org:8080/#/c/13049/4/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@345 PS4, Line 345: eventsProcessor_.processEvents(); > I feel we should add an additional test, here we are testing if the Notific Done http://gerrit.cloudera.org:8080/#/c/13049/4/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@1481 PS4, Line 1481: } > Can we check if possible that the counter MetastoreEventsProcessor.EVENTS_S Done -- 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: 5 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: Fri, 26 Apr 2019 22:47:44 +0000 Gerrit-HasComments: Yes
