Tim Armstrong 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 14: (5 comments) I'm not planning on doing a full review since Bharath and Paul are on it. The concurrency aspects I looked at are much better in this version. I had some follow on questions about the interaction between the initial load of the catalog and the invalidation - I think you anticipated that to some extent. If it's too much to address in this patch we could consider merging this without addressing that but keeping it behind an experimental feature flag until some more of the concerns have been addressed. http://gerrit.cloudera.org:8080/#/c/12118/14/be/src/common/global-flags.cc File be/src/common/global-flags.cc: http://gerrit.cloudera.org:8080/#/c/12118/14/be/src/common/global-flags.cc@245 PS14, Line 245: DEFINE_int32(hms_event_polling_frequency_s, 0, Maybe make it hidden or mention that it's experimental in the text here if it's not ready for prime time yet? http://gerrit.cloudera.org:8080/#/c/12118/14/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/14/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@299 PS14, Line 299: * Initializes Metastore event processor object if This occurs before the initial metadata is loaded in reset(), right? So we might process some events that occurred before we got an initial snapshot of the data. I think this is probably OK if the events are idempotent, but seems worth mentioning in the comment. Actually one question is how this interacts with reset(), either the initial reset() on startup or a later one from "INVALIDATE METADATA". What happens if events start to be processed when the catalog is empty before reset() loads all the data? And should we reset the current event ID when doing an invalidate? I'm wondering if those questions lead us to resetting currentNotificationId in reset() and stopping/pausing the even processing until reset() completes. That would at least reduce the number of possible races. http://gerrit.cloudera.org:8080/#/c/12118/14/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@319 PS14, Line 319: LOG.fatal("Unable to fetch the current notification event id from metastore." This does actually terminate the process - it call to the glog LOG(FATAL) which calls abort(). I think we don't want this since it generates a core dump - Maybe LOG.error() + throwing an exception is the right behaviour. We've had issues before with misconfigured catalogds generating a ton of core dumps. http://gerrit.cloudera.org:8080/#/c/12118/14/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1342 PS14, Line 1342: null ? http://gerrit.cloudera.org:8080/#/c/12118/14/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/14/fe/src/main/java/org/apache/impala/catalog/MetastoreEventsProcessor.java@169 PS14, Line 169: // TODO figure out if CatalogD sync with HMS is in-process or completed so that we Ah, I think this is referring to the same kind of issue that I mentioned earlier with reset() running in parallel with this thread. -- 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: 14 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: Fri, 11 Jan 2019 00:03:21 +0000 Gerrit-HasComments: Yes
