Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12118 )
Change subject: IMPALA-7970 : Add support for automatic invalidates by polling metastore events ...................................................................... Patch Set 8: (3 comments) I took a look at the locking aspect of this (given the history of the challenges there, we'll want multiple pairs of eyes on that aspect) http://gerrit.cloudera.org:8080/#/c/12118/8//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/12118/8//COMMIT_MSG@18 PS8, Line 18: Can you summarise how this integrates with the existing catalog locking protocol? That's not mentioned in the design doc. http://gerrit.cloudera.org:8080/#/c/12118/8/fe/src/main/java/org/apache/impala/catalog/Db.java File fe/src/main/java/org/apache/impala/catalog/Db.java: http://gerrit.cloudera.org:8080/#/c/12118/8/fe/src/main/java/org/apache/impala/catalog/Db.java@112 PS8, Line 112: synchronized (thriftDb_) { thriftDb_.setMetastore_db(msDb); } This additional ad-hoc locking without explanation of how it integrates with the other concurrency control in the catalog (locks, version numbers, etc) is concerning. If threads are updating this concurrently without using the protocol around versionLock_, that's probably going to mess up the whole protocol, since you'll see different thriftDb_ values for the same version number. http://gerrit.cloudera.org:8080/#/c/12118/8/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/8/fe/src/main/java/org/apache/impala/catalog/MetastoreEventsProcessor.java@305 PS8, Line 305: catalog_.removeTable(oldTable.getDbName(), oldTable.getTableName()); Bharath mentioned it, but there's definitely a race here - you can see that removeTable() drops the lock before returning so this operation isn't atomic - there's a window where it would appear to another thread that the table disappeared. -- 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: 8 Gerrit-Owner: Anonymous Coward <[email protected]> Gerrit-Reviewer: Anonymous Coward <[email protected]> Gerrit-Reviewer: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Wed, 26 Dec 2018 17:34:44 +0000 Gerrit-HasComments: Yes
