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 21: (14 comments) Patch code flow and logic lgtm generally. Just had some nits/clarifications. Publishing these while I'm looking at the tests. http://gerrit.cloudera.org:8080/#/c/12118/21/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/21/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@307 PS21, Line 307: BackendConfig.INSTANCE.getHMSPollingIntervalInSeconds() eventPollingInterval http://gerrit.cloudera.org:8080/#/c/12118/21/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@319 PS21, Line 319: LOG.error("Unable to fetch the current notification event id from metastore." : + "Metastore event processing will be disabled.", : e); nit: Multiple places in the code, try to format in fewer lines. Something like LOG.error("......." + ".....", e) http://gerrit.cloudera.org:8080/#/c/12118/21/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1206 PS21, Line 1206: // Even though we get the current notification event id before stopping the event clarification: reset() is equivalent to invalidating everything and fetching from scratch from HMS. In that case, do we still restart the event processing from where we left off (before reset()) or can we start with the latest event ID from HMS? http://gerrit.cloudera.org:8080/#/c/12118/21/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1804 PS21, Line 1804: Table incompleteTable = IncompleteTable.createUninitializedTable(db, tblName); move it after L1807? http://gerrit.cloudera.org:8080/#/c/12118/13/fe/src/main/java/org/apache/impala/catalog/MetastoreEventsProcessor.java File fe/src/main/java/org/apache/impala/catalog/MetastoreEventsProcessor.java: http://gerrit.cloudera.org:8080/#/c/12118/13/fe/src/main/java/org/apache/impala/catalog/MetastoreEventsProcessor.java@195 PS13, Line 195: > curious to understand why do you think this method has a race? nvm missed the synchronized part. http://gerrit.cloudera.org:8080/#/c/12118/21/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/21/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventUtils.java@155 PS21, Line 155: event %d of type %s on table %s I think it is cleaner to define a debugString(NotificationEvent event) {} that formats it cleanly and consistently across all invocations. We can use it to dump event state in multiple places. http://gerrit.cloudera.org:8080/#/c/12118/21/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventUtils.java@160 PS21, Line 160: if (!event.getEventType().equalsIgnoreCase("CREATE_DATABASE") : && !event.getEventType().equalsIgnoreCase("DROP_DATABASE")) { : tblName_ = Preconditions.checkNotNull(event.getTableName()); : } else { : tblName_ = null; : } nit: Reformat to the following? (fewer branches and use enum) tblName_ = null; if (eventType_ == MetaStoreEventType.CREATE_TABLE) { tblName_ = ... } http://gerrit.cloudera.org:8080/#/c/12118/21/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventUtils.java@181 PS21, Line 181: tblName_ checkNotNull()? http://gerrit.cloudera.org:8080/#/c/12118/21/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventUtils.java@198 PS21, Line 198: private final String dbName_; : private final String tblName_; aren't these set in the parent c'tor? http://gerrit.cloudera.org:8080/#/c/12118/21/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventUtils.java@347 PS21, Line 347: droppedTable_ isn't this more like tblTobeDropped_ ? http://gerrit.cloudera.org:8080/#/c/12118/21/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventUtils.java@434 PS21, Line 434: // nit: remove, you are already in a comment block. http://gerrit.cloudera.org:8080/#/c/12118/21/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/21/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java@310 PS21, Line 310: if (currentEventId <= lastSyncedEventId_) { : return Collections.emptyList(); : } nit: single line http://gerrit.cloudera.org:8080/#/c/12118/21/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java@334 PS21, Line 334: // need to take a write lock since event status needs to be updated in case of errors : // TODO this can improved by taking a readlock here and adding a error handling : // logic in the catch clause which takes a writelock and then downgrades to the : // readlock again. The added complexity is not necessary at this time but we may : // have to revisit this in the future This blocks invalidate metadata since both of them try to get a writelock and the scope of the write lock here is too big and we are also doing RPCs under lock. We had nasty issues around such locks in the past. Any chance you could narrow down the scope or fix this? http://gerrit.cloudera.org:8080/#/c/12118/21/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java@373 PS21, Line 373: .append("Event id : " + event.getEventId() + "\n") weird formatting. -- 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: 21 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: Fri, 18 Jan 2019 16:30:22 +0000 Gerrit-HasComments: Yes
