Paul Rogers has posted comments on this change. ( http://gerrit.cloudera.org:8080/12591 )
Change subject: IMPALA-7972 Detect self-events to avoid unnecessary invalidates ...................................................................... Patch Set 4: (7 comments) Identified one more potential race condition and suggested a solution. http://gerrit.cloudera.org:8080/#/c/12591/4/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/12591/4/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@738 PS4, Line 738: public Collection<Long> getInFlightVersionsForEvents(String dbName, String tblName) Should be a list: evens are ordered and must be ignored in the order that they were added to the list. http://gerrit.cloudera.org:8080/#/c/12591/4/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@765 PS4, Line 765: public void removeFromInFlightVersionsForEvents(String dbName, String tblName, This form introduces a race condition. Can you do something like: void invaliateOrIgnore(int versionNo) - lock - if list is non-empty, and first entry is versionNo, remove that first entry - Else, invalidate the table - unlock http://gerrit.cloudera.org:8080/#/c/12591/4/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java File fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java: http://gerrit.cloudera.org:8080/#/c/12591/4/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@889 PS4, Line 889: protected Collection<Long> pendingVersionNumbersFromCatalog_ = Collections.EMPTY_LIST; Again, this introduces a race condition if some other thread changes the table since we obtained the list. http://gerrit.cloudera.org:8080/#/c/12591/4/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@919 PS4, Line 919: protected boolean isSelfEvent() throws CatalogException { I see. You want to identify outside of a lock if this is a self event. The proposed non-racy solution makes self-event detection an integral part of the invalidate, avoiding race conditions. The logic would be: if server id not set, or does not match - invalidate else - invalidOrIgnore(event version) Can even be simplified to: eventVersion = -1 if serverId is set and matches then evenVersion = version from event invalidateOrIgnore(eventVersion) I think this completely eliminates all race condition paths http://gerrit.cloudera.org:8080/#/c/12591/4/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@957 PS4, Line 957: if (isSelfEvent()) { To enable this logging, have invalidOrIgnore return true if ignored, false if invalidated (or visa-versa, you choose) http://gerrit.cloudera.org:8080/#/c/12591/4/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@1100 PS4, Line 1100: protected boolean isSelfEvent() { With the new system, we don't know if it is a self event until we lock and look. All we can tell is that, when we checked, it WAS a self event. http://gerrit.cloudera.org:8080/#/c/12591/4/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/12591/4/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java@148 PS4, Line 148: * its a performance penalty not a correctness issue. Thanks for the explanation! -- To view, visit http://gerrit.cloudera.org:8080/12591 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6db0d7f7fe465158fc8cb9d6b6b57a321827b353 Gerrit-Change-Number: 12591 Gerrit-PatchSet: 4 Gerrit-Owner: Vihang Karajgaonkar <vih...@cloudera.com> Gerrit-Reviewer: Bharath Krishna <bhar...@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: Vihang Karajgaonkar <vih...@cloudera.com> Gerrit-Comment-Date: Wed, 27 Feb 2019 23:13:21 +0000 Gerrit-HasComments: Yes
