Sai Hemanth Gantasala has posted comments on this change. ( http://gerrit.cloudera.org:8080/19484 )
Change subject: IMPALA-11822: Optimize the Refresh/Invalidate event processing by skipping unnecessary events ...................................................................... Patch Set 3: (8 comments) http://gerrit.cloudera.org:8080/#/c/19484/2/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/19484/2/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@300 PS2, Line 300: > I think we need ConcurrentHashMap since this will be updated in multi-threa Ack http://gerrit.cloudera.org:8080/#/c/19484/1/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/19484/1/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@2516 PS1, Line 2516: > Sorry, what I mean is clock skew between nodes. It's possible that the HMS Ok, Because of the clock skew, (I can think of one example where) we can (possibly) end up processing an older event. And this is inevitable and not desirable. (I cannot think of an example where we miss the event because of clock skew). Not sure how can overcome this problem. http://gerrit.cloudera.org:8080/#/c/19484/1/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java: http://gerrit.cloudera.org:8080/#/c/19484/1/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@6362 PS1, Line 6362: Reference<Boolean> dbWasAdded = new Reference<Boolean>(false); > Oh, I just realized the map is updated in fireReloadEventHelper. Can we upd Ack http://gerrit.cloudera.org:8080/#/c/19484/1/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@6423 PS1, Line 6423: throw new TableNotFoundException("Table not found: " + > nit: need space after '//' Ack http://gerrit.cloudera.org:8080/#/c/19484/1/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@6434 PS1, Line 6434: d as a direct DDL operation. : if (tblWasRemoved.getRef()) { > nit: need spaces after '//', 'for' and around ':' Ack http://gerrit.cloudera.org:8080/#/c/19484/2/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java: http://gerrit.cloudera.org:8080/#/c/19484/2/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@4513 PS2, Line 4513: /** > It seems we don't need to set the map back. It's updated in place. Ack http://gerrit.cloudera.org:8080/#/c/19484/2/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@6425 PS2, Line 6425: req.getTable_name().getTable_name()); > Why do we need to create a new map here? Can we directly modify the map cor Ack http://gerrit.cloudera.org:8080/#/c/19484/2/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@6444 PS2, Line 6444: Db addedDb = catalog_.getDb(updatedThriftTable.getTable().getDb_name()); > This might have concurrent issue when there are concurrent refreshes on dif Ack -- To view, visit http://gerrit.cloudera.org:8080/19484 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I905957683a96c3ea01ab4bf043d6658ce37b7574 Gerrit-Change-Number: 19484 Gerrit-PatchSet: 3 Gerrit-Owner: Sai Hemanth Gantasala <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Sai Hemanth Gantasala <[email protected]> Gerrit-Comment-Date: Fri, 03 Mar 2023 03:35:50 +0000 Gerrit-HasComments: Yes
