Bharath Krishna 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 7: (14 comments) Thanks for the review Vihang. Updated patch with comments resolved. http://gerrit.cloudera.org:8080/#/c/12938/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/12938/4//COMMIT_MSG@8 PS4, Line 8: incorrect deletion of Database > I think this is not accurate. creation_time is not used for invalidating bu Done http://gerrit.cloudera.org:8080/#/c/12938/4//COMMIT_MSG@15 PS4, Line 15: we can skip the event : instead. > suggest you to change it to ".. and we can skip the event instead" which is Done http://gerrit.cloudera.org:8080/#/c/12938/4/be/src/util/event-metrics.h File be/src/util/event-metrics.h: http://gerrit.cloudera.org:8080/#/c/12938/4/be/src/util/event-metrics.h@40 PS4, Line 40: static DoubleGauge* EVENTS_FETCH_DURATION_MEAN; > Is there any specific value in exposing this metric on the Catalog metrics Done http://gerrit.cloudera.org:8080/#/c/12938/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/12938/4/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@953 PS4, Line 953: public List<String> getTableProperties( > like suggested in my other comment related to race conditions , this API in Removed this method http://gerrit.cloudera.org:8080/#/c/12938/4/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/4/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@208 PS4, Line 208: metrics_.getCounter(MetastoreEventsProcessor.EVENTS_FILTERED_METRIC) > Just having this line is sufficient to expose the metric in /events page. Y Done http://gerrit.cloudera.org:8080/#/c/12938/4/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@898 PS4, Line 898: vent's DB object, it means that the Database object present > The process method does not invalidate so this description needs to be upda Done http://gerrit.cloudera.org:8080/#/c/12938/4/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@899 PS4, Line 899: * in the catalog is a later version and we can skip the event. > line too long (100 > 90) Done http://gerrit.cloudera.org:8080/#/c/12938/4/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@903 PS4, Line 903: verride : public void process() { : Reference<Boolean> dbFound = new Reference<>(); : Reference<Boolean> dbMatched = new Reference<>(); : Db removedDb = catalog_.removeDbIfExists(droppedDatabase_, dbFound, dbMatched); : if (removedDb != null) { : infoLog("Removed Database {} ", dbName_); : } else if (!dbMatched.getRef()) { : LOG.warn(debugString("Database %s was not removed from catalog since " : + "the creation time of the Database did not match", dbName_)); : metrics_.getCounter(MetastoreEventsProcessor.EVENTS_FILTERED_METRIC).inc(); : } else if (!dbFound.getRef()) { : debugLog("Database {} was not removed since it did not exist in catalog > This code has potential race conditions. For example, the createTime of the Refactored this method similar to DropTable. Now atomically checking CREATE_TIME and removing DB http://gerrit.cloudera.org:8080/#/c/12938/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/12938/4/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@272 PS4, Line 272: * DROP_DATABASE uses CREATION_TIME to filter events that try to drop an > add javadoc to describe what exactly this test is testing. Specifically, de Done http://gerrit.cloudera.org:8080/#/c/12938/4/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@284 PS4, Line 284: > add javadoc Done http://gerrit.cloudera.org:8080/#/c/12938/4/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@285 PS4, Line 285: // Check that the database exists in Catalog > line too long (126 > 90) Done http://gerrit.cloudera.org:8080/#/c/12938/4/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@293 PS4, Line 293: */ > line too long (91 > 90) Done http://gerrit.cloudera.org:8080/#/c/12938/4/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@298 PS4, Line 298: createDatabaseFromImpala(TEST_DB_NAME, "Test DB for CREATION_TIME"); > hmm, when a create_db, drop_db, create_db is found, I thought only the firs I have a comment explaining why count=2. First it filters CREATE_DATABASE as there is another one with the same name. Second, it filters DROP_DATABASE based on CREATION_TIME. http://gerrit.cloudera.org:8080/#/c/12938/4/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@302 PS4, Line 302: createDatabaseFromImpala(TEST > isn't the @After method already doing this? I only saw AfterClass method which does this. So keeping it there. -- 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: 7 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: Wed, 10 Apr 2019 01:02:29 +0000 Gerrit-HasComments: Yes
