Anurag Mantripragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/14145 )
Change subject: IMPALA-8579: Ignore trivial alter table/partition events. ...................................................................... Patch Set 2: (7 comments) Thanks for the comments, Vihang. Addressed your comments. Also, for the sake of uniformity I used "parameters" everywhere instead of "properties". http://gerrit.cloudera.org:8080/#/c/14145/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/14145/1/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@944 PS1, Line 944: etermine whether this is > may be a more boring name like canBeSkipped()? :) Done http://gerrit.cloudera.org:8080/#/c/14145/1/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@945 PS1, Line 945: > this log could be more descriptive since most likely a reader of that state Done http://gerrit.cloudera.org:8080/#/c/14145/1/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@1023 PS1, Line 1023: > Do we need this check? If not, you can change this to a static util method We probably don't need this check. However we will need to compare full table objects. http://gerrit.cloudera.org:8080/#/c/14145/1/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@1030 PS1, Line 1030: er table event is such an event and can be skipped.. : */ > I think we should not change the state of the table object from the event. Makes sense. Will make a copy of the objects. http://gerrit.cloudera.org:8080/#/c/14145/1/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@1368 PS1, Line 1368: > This list could be a static final Immutable list since we don't expect it t Done http://gerrit.cloudera.org:8080/#/c/14145/1/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@1569 PS1, Line 1569: > Do we need this check? Done http://gerrit.cloudera.org:8080/#/c/14145/1/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/14145/1/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@160 PS1, Line 160: > Another reason to use a static final constant which can be reused. Such imp Done -- To view, visit http://gerrit.cloudera.org:8080/14145 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I01a59d5170accc014f76f14eb526d96ddcf61f76 Gerrit-Change-Number: 14145 Gerrit-PatchSet: 2 Gerrit-Owner: Anurag Mantripragada <anu...@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-Comment-Date: Tue, 27 Aug 2019 06:51:05 +0000 Gerrit-HasComments: Yes
