Quanlong Huang 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 2: (9 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: HashMap I think we need ConcurrentHashMap since this will be updated in multi-threads. 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: > Yeah, the event time is generated by HMS. This variable stores the latest t Sorry, what I mean is clock skew between nodes. It's possible that the HMS and catalogd nodes have several milliseconds difference. If any of them have connection issues with the ntp server, the time difference could be even larger. 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: } > Yeah, I did cover the normal refresh cases as well. Did I miss anything? Oh, I just realized the map is updated in fireReloadEventHelper. Can we update the method name to reflect this, e.g. fireReloadEventAndUpdateRefreshMap? http://gerrit.cloudera.org:8080/#/c/19484/1/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@6423 PS1, Line 6423: //Update the entries in the refresh metadata map nit: need space after '//' http://gerrit.cloudera.org:8080/#/c/19484/1/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@6434 PS1, Line 6434: //update all the partitions values : for(String partitionName: nit: need spaces after '//', 'for' and around ':' 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: catalog_.setRefreshMetadataMap(refreshMetadataMap); It seems we don't need to set the map back. It's updated in place. http://gerrit.cloudera.org:8080/#/c/19484/2/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@6398 PS2, Line 6398: catalog_.setRefreshMetadataMap(refreshMetadataMap); I think we can simply set an empty map and don't need to get and clear the current map. Or if we get and clear the current map, we don't need to set it back. http://gerrit.cloudera.org:8080/#/c/19484/2/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@6425 PS2, Line 6425: new HashMap<>(catalog_.getRefreshMetadataMap()); Why do we need to create a new map here? Can we directly modify the map corresponding to the table? http://gerrit.cloudera.org:8080/#/c/19484/2/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@6444 PS2, Line 6444: catalog_.setRefreshMetadataMap(refreshMetadataMap); This might have concurrent issue when there are concurrent refreshes on different tables. -- 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: 2 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: Sat, 25 Feb 2023 05:29:13 +0000 Gerrit-HasComments: Yes
