Vihang Karajgaonkar 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: (10 comments) 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. Done 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 Done 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 sil drop events are implemented as dropIfExists. So since the create event is skipped, the drop event processing becomes a no-op. I wanted to remove the drop event as well, but it was complicating this code here without any added benefit. I will add a comment here saying why its ok to not remove the drop event so that its clear in the future. 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 Good point. Also, added logging in the isRemovedAfter impl for createTable and createDatabase events. 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(); Actually, added a log statement in the if block so can't do this anymore. 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? Done 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. this statement cannot be converted to a 2 line statement without increasing the line width beyond 100 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. Reduced the text verbosity to make it to 2 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 > ? had kept a reminder for myself to add partition and alter partition test. Added them. Thanks for pointing this out. http://gerrit.cloudera.org:8080/#/c/12118/25/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@583 PS25, Line 583: in > ? updated the doc. -- 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 <vih...@cloudera.com> Gerrit-Reviewer: Bharath Vissapragada <bhara...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Paul Rogers <prog...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Vihang Karajgaonkar <vih...@cloudera.com> Gerrit-Comment-Date: Tue, 29 Jan 2019 01:18:33 +0000 Gerrit-HasComments: Yes
