Vihang Karajgaonkar has posted comments on this change. ( http://gerrit.cloudera.org:8080/14145 )
Change subject: IMPALA-8579: Ignore trivial alter table/partition events. ...................................................................... Patch Set 1: (7 comments) 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: isTrivialAlterTableEvent may be a more boring name like canBeSkipped()? :) http://gerrit.cloudera.org:8080/#/c/14145/1/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@945 PS1, Line 945: it is a trivial alter event this log could be more descriptive since most likely a reader of that statement would not understand why this event was ignored. May be something like, "Skipping this event since it only alters some table properties which can be ignored". http://gerrit.cloudera.org:8080/#/c/14145/1/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@1023 PS1, Line 1023: !tableAfter_.equals(tableBefore_) Do we need this check? If not, you can change this to a static util method which takes in two maps for the beforeProperties and afterProperties http://gerrit.cloudera.org:8080/#/c/14145/1/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@1030 PS1, Line 1030: tableAfter_.getParameters().put(property, : tableBefore_.getParameters().get(property)); I think we should not change the state of the table object from the event. You could either create a copy and operate on it or use a try .. finally block to restore the original values of the table properties. This may be an issue today but we can run into weird bugs in the future if we decide to make use of these objects to update the catalog state directly in the future. http://gerrit.cloudera.org:8080/#/c/14145/1/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@1368 PS1, Line 1368: getPropertiesToIgnore This list could be a static final Immutable list since we don't expect it to change from event type http://gerrit.cloudera.org:8080/#/c/14145/1/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@1569 PS1, Line 1569: !partitionAfter_.equals(partitionBefore_) Do we need this check? 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: alterPropertiesToIgnore_ Another reason to use a static final constant which can be reused. Such implementations are buggy since the main code could change while this test keeps passing. -- 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: 1 Gerrit-Owner: Anurag Mantripragada <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Vihang Karajgaonkar <[email protected]> Gerrit-Comment-Date: Tue, 27 Aug 2019 03:11:34 +0000 Gerrit-HasComments: Yes
