Bharath Vissapragada 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 10: (13 comments) A bunch of nits but otherwise lgtm. Can +2 once those are fixed. http://gerrit.cloudera.org:8080/#/c/12938/10/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/10/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1536 PS10, Line 1536: ..does not exist or not removed because ctime matches.. http://gerrit.cloudera.org:8080/#/c/12938/10/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1539 PS10, Line 1539: Reference<Boolean> dbFound, Reference<Boolean> dbMatched) { nit: indentation http://gerrit.cloudera.org:8080/#/c/12938/10/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1548 PS10, Line 1548: // Make sure we are removing the same instance of Database by : // checking the CREATION_TIME of the msDb with that of Catalog's DB and : // remove the DB from Catalog only if the CREATION_TIME is equal. Simplify to ..Remove the db only if the ctime matches with msDB... ? http://gerrit.cloudera.org:8080/#/c/12938/10/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1552 PS10, Line 1552: Db removedDb = super.removeDb(dbName); : if (removedDb != null) { : updateDeleteLog(removedDb); : dbMatched.setRef(true); : } use this.removeDb() which does all of this for you. http://gerrit.cloudera.org:8080/#/c/12938/10/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/10/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@874 PS10, Line 874: CatalogServiceCatalog catalog, Metrics metrics, NotificationEvent event) nit: 4 space indentation http://gerrit.cloudera.org:8080/#/c/12938/10/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@885 PS10, Line 885: throw new MetastoreNotificationException(debugString( nit: 4 space indentation http://gerrit.cloudera.org:8080/#/c/12938/10/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@886 PS10, Line 886: "Database object is null in the event. Is this the only issue that can cause an "Exception"? Either make it generic and say something like "Error parsing the event info..." or handle the right exception, try { } catch (DbNullCausingException foo) { log(Db is null); } catch (Exception e) { log(error parsing...) } http://gerrit.cloudera.org:8080/#/c/12938/10/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@909 PS10, Line 909: infoLog("Removed Database {} ", dbName_); nit: 2space indentation http://gerrit.cloudera.org:8080/#/c/12938/10/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@912 PS10, Line 912: "did not exist in catalog.", dbName_); nit: 4 space indentation http://gerrit.cloudera.org:8080/#/c/12938/10/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@915 PS10, Line 915: + "the creation time of the Database did not match", dbName_)); nit: 4 space indentation http://gerrit.cloudera.org:8080/#/c/12938/10/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/10/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@275 PS10, Line 275: Create_DB nit: CREATE_DB http://gerrit.cloudera.org:8080/#/c/12938/10/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@283 PS10, Line 283: nit: remove unnecessary spaces http://gerrit.cloudera.org:8080/#/c/12938/10/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@296 PS10, Line 296: public void testDropDatabaseCreationTime() indentation off in multiple places. -- 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: 10 Gerrit-Owner: Bharath Krishna <[email protected]> Gerrit-Reviewer: Anurag Mantripragada <[email protected]> Gerrit-Reviewer: Bharath Krishna <[email protected]> Gerrit-Reviewer: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Vihang Karajgaonkar <[email protected]> Gerrit-Comment-Date: Sun, 14 Apr 2019 18:43:25 +0000 Gerrit-HasComments: Yes
