Vihang Karajgaonkar has posted comments on this change. ( http://gerrit.cloudera.org:8080/12938 )
Change subject: IMPALA-8338 : Check CREATION_TIME in event processor to avoid incorrect deletion of Database objects. ...................................................................... Patch Set 8: (5 comments) Patch mostly looks good to me. I have one suggestion related to metrics below. http://gerrit.cloudera.org:8080/#/c/12938/7/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/12938/7/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@911 PS7, Line 911: bugLog("Database {} change to infoLog since this is not a warning and cause of concern. http://gerrit.cloudera.org:8080/#/c/12938/8/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/12938/8/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@208 PS8, Line 208: EVENTS_FILTERED_METRIC see my comment regarding have a new metric which represent almost the same thing as events_skipped below. http://gerrit.cloudera.org:8080/#/c/12938/8/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@916 PS8, Line 916: EVENTS_FILTERED_METRIC its not super clear on the difference between events_filtered and events_skipped. I think we should either merge them together or for now, we should use events_skipped here. http://gerrit.cloudera.org:8080/#/c/12938/8/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java File fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java: http://gerrit.cloudera.org:8080/#/c/12938/8/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java@202 PS8, Line 202: EVENTS_FILTERED_METRIC Its confusing to have a events_filtered and events_skipped (not to mention self_events which are also skipped). Perhaps we should merge them into one. So we may get rid of this metric and use EVENTS_SKIPPED_METRIC instead? If you do that also update the comment above EVENTS_SKIPPED_METRIC http://gerrit.cloudera.org:8080/#/c/12938/8/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/12938/8/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@300 PS8, Line 300: dropDatabaseCascadeFromImpala(TEST_DB_NAME); : // Create database again with same name : createDatabaseFromImpala(TEST_DB_NAME, "Test DB for CREATION_TIME"); : eventsProcessor_.processEvents(); this could be flaky if the drop and create happened in the same second. Do you want to add a sleep(2sec) here and mention that this is a limitation. -- To view, visit http://gerrit.cloudera.org:8080/12938 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8fd3685cf7261e4953f4f884850489a47c5bbd6c Gerrit-Change-Number: 12938 Gerrit-PatchSet: 8 Gerrit-Owner: Bharath Krishna <[email protected]> Gerrit-Reviewer: Anurag Mantripragada <[email protected]> Gerrit-Reviewer: Bharath Krishna <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Vihang Karajgaonkar <[email protected]> Gerrit-Comment-Date: Thu, 11 Apr 2019 19:43:14 +0000 Gerrit-HasComments: Yes
