Bharath Vissapragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/12118 )
Change subject: IMPALA-7970 : Add support for metastore event based automatic invalidate ...................................................................... Patch Set 25: (12 comments) lgtm. Final round of comments from my side. Given the state the patch is in, I think we should get the patch into the master branch which makes it easier for concurrency testing. Also since all of this code is behind a feature flag, it should not affect any existing deployments. http://gerrit.cloudera.org:8080/#/c/12118/25/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/12118/25/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@308 PS25, Line 308: LOG.info(String.format("Metastore event processing is disabled. Event polling " nit: fix formatting. http://gerrit.cloudera.org:8080/#/c/12118/25/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1833 PS25, Line 1833: if (db == null) return null; : if (!db.containsTable(tblName)) return null nit: merge into a single stmt. http://gerrit.cloudera.org:8080/#/c/12118/25/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventUtils.java File fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventUtils.java: http://gerrit.cloudera.org:8080/#/c/12118/25/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventUtils.java@145 PS25, Line 145: no not http://gerrit.cloudera.org:8080/#/c/12118/25/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventUtils.java@149 PS25, Line 149: * removed a table, the create event received will try to add the object again. This probably needs some thought during concurrency testing. http://gerrit.cloudera.org:8080/#/c/12118/25/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventUtils.java@166 PS25, Line 166: drop event later Do we also need to remove the drop event or are we just letting it fail silently? http://gerrit.cloudera.org:8080/#/c/12118/25/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventUtils.java@174 PS25, Line 174: fromIndex++; Log how many events got filtered out? Helps diagnosing incase there are any bugs in this area. http://gerrit.cloudera.org:8080/#/c/12118/25/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventUtils.java@356 PS25, Line 356: if (this.dbName_.equalsIgnoreCase(dropTableEvent.dbName_) && this.tblName_ : .equalsIgnoreCase(dropTableEvent.tblName_)) { : return true; : } nit: simplify to return dbName_.equals() && tblName_.equals(); http://gerrit.cloudera.org:8080/#/c/12118/25/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventUtils.java@360 PS25, Line 360: else if (event.eventType_.equals(MetastoreEventType.ALTER_TABLE)) { : // if this create table was renamed to some other name in the future return true : AlterTableEvent alterTableEvent = (AlterTableEvent) event; : if (alterTableEvent.isRename_ && this.dbName_ : .equalsIgnoreCase(alterTableEvent.tableBefore_.getDbName()) && this.tblName_ : .equalsIgnoreCase(alterTableEvent.tableBefore_.getTableName())) { : return true; : } clarify why rename qualifies as an inverse op for create? http://gerrit.cloudera.org:8080/#/c/12118/25/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/12118/25/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java@295 PS25, Line 295: LOG.info(String : .format("Received %d events. Start event id : %d", response.getEvents().size(), : lastSyncedEventId_)); nit: format to fewer lines. http://gerrit.cloudera.org:8080/#/c/12118/25/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java@319 PS25, Line 319: LOG.warn(String.format( : "Not processing notification events since event processing status " : + "is %s. Last synced event id is %d", currentStatus, : lastSyncedEventId_)); nit: format to fewer lines. http://gerrit.cloudera.org:8080/#/c/12118/25/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/12118/25/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@339 PS25, Line 339: : // remove some partitions : // change some partitions ? http://gerrit.cloudera.org:8080/#/c/12118/25/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@583 PS25, Line 583: in ? -- To view, visit http://gerrit.cloudera.org:8080/12118 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic70b27780560b7ac9b33418d132b36cd0ca4abf7 Gerrit-Change-Number: 12118 Gerrit-PatchSet: 25 Gerrit-Owner: Vihang Karajgaonkar <[email protected]> Gerrit-Reviewer: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Paul Rogers <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Reviewer: Vihang Karajgaonkar <[email protected]> Gerrit-Comment-Date: Mon, 28 Jan 2019 23:23:59 +0000 Gerrit-HasComments: Yes
