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 22: (5 comments) I forgot to publish these comments earlier. Publishing it here again just for the record. http://gerrit.cloudera.org:8080/#/c/12118/20/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/20/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventUtils.java@383 PS20, Line 383: > Nit: Done http://gerrit.cloudera.org:8080/#/c/12118/22/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/22/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventUtils.java@360 PS22, Line 360: throw new MetastoreNotificationException(debugString( > To follow up on previous comments about errors. We may have the user busily Thats a fair point. Let me add more details on what can go wrong during this event processing. I would analyze what could be state of catalog at the event processing time based on the diagram I have in the event processor class. Case I: Catalog state is stale with respect to this event. a. It knows of a table with oldTblName but doesn't know about newTblName This maps to the regular renameTable by remove oldTbl and add newTbl. Case II: Catalog state is exactly at the same state as of event. a. This means that catalog doesn't know of oldTbl. It knows about newTbl. In this case also renameTable fails since it cannot find oldTbl. However, we can continue event processing. Basically, no action would be taken by this event and it needs to be ignored. Case III: Catalog state is ahead of event. This could mean a number of things. a. simple case is nothing exotic happens. OldTbl doesn't exist, newTbl exists --> case II b. user did a reverse rename from newtbl -> oldTbl post event. But we don't have any mechanism currently to distinguish this state from Case I without the version number support. c. User dropped the newTbl (or renamed it to something else). In this case, event processor will neither see a oldTbl or newTbl in the cache. It looks like there would be slight window of time where newTbl will appear until the actual event of the newTbl is processed and it is dropped (or renamed). Given these various possibilities, I think the right thing to do is Atomic rename by drop-and-add 1. remove oldTbl if it exists. 2. add newTbl if it doesn't exist. To compare a existing tbl we should use the creationTime and version. http://gerrit.cloudera.org:8080/#/c/12118/22/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventUtils.java@515 PS22, Line 515: LOG.info(debugString("Successfully removed database %s", dbName_)); > Briefly looked at these other error handling blocks. The exception/logging Thanks for taking a relook 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@334 PS21, Line 334: lastSyncedEventId_ = event.getEventId(); : } : } : } catch (MetastoreNotificationException | CatalogException ex) { : updateStatus(EventProcessorStatus.E > This blocks invalidate metadata since both of them try to get a writelock a changed the locking logic to use simple synchronized blocks. The code becomes a little more ugly but reduces the total blocking time for reset() as low as possible. http://gerrit.cloudera.org:8080/#/c/12118/22/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/22/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java@258 PS22, Line 258: public synchronized void start(long fromEventId) { > This is synchronized, which suggests that we might get multiple concurrent Both the start() and stop() methods do a Preconditions.checkState(). In this particular start method the check is on line 261. I will move it to the first couple of lines to improve readability. -- 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: 22 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:24:33 +0000 Gerrit-HasComments: Yes
