Paul Rogers 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: (3 comments) Thanks much for your great work in getting this in and helping us anticipate all the ways that things might go wrong in a production system. Thanks much also for discussing the code review comments in person. This review has just a few minor follow-on comments. After that, I suspect we'll be in good shape to merge the code: we are reaching the point of diminishing returns in reviews; the next step is to try the feature out in a live cluster, and for that you'll need the code merged. A feature flag lets us turn off the feature if we encounter anything missed during reviews. 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 hammering the system with INVALIDATEs and queries, each of which may mutate the cache ahead of the event processing here. If we encounter such a case, we should soldier on rather than stopping event processing. So, the case that the old table does not exist, is that an expected error if the user issues an INVALIDATE? Or, does it mean that we have a likely bug and we should stop event processing? The case of the missing new table is different: it would seem that there would be no good reason not to add the new table. But, I wonder, does the renameTable() method handle the case in which the rename was already done? Something like: * Rename table in HMS. * Issue a query against the new name, causing the catalog to load metadata for the new table. * Get the rename event. * Find the old table and remove it, but be unable to add a new table since a table of that name was already loaded. 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 decisions look right on each of them. 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 start/stop calls from different threads. Should the start() (and stop()) methods check if they are in the proper state before proceeding? That is, should this method check if we are already in the ACTIVE state and simply return if so? -- 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: Wed, 23 Jan 2019 21:18:50 +0000 Gerrit-HasComments: Yes
