Zoltan Borok-Nagy 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: (7 comments) Left some comments mostly about style, but looks good overall. http://gerrit.cloudera.org:8080/#/c/19484/3/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/3/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@3876 PS3, Line 3876: public void setRefreshMetadataMap(Map<String, Map<String, Integer>> metadataMap) { : refreshMetadataMap.set(metadataMap); : } Seems unused, in which case there is no need for the AtomicReference. http://gerrit.cloudera.org:8080/#/c/19484/3/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@3881 PS3, Line 3881: nit: needs +4 spaces instead of +2 http://gerrit.cloudera.org:8080/#/c/19484/3/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@3890 PS3, Line 3890: nit: needs +4 spaces instead of +2 http://gerrit.cloudera.org:8080/#/c/19484/3/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/3/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@2534 PS3, Line 2534: Map<String, Integer> partitionMap = : catalog_.getPartitionMapFromRefreshMap(dbName_, tblName_); nit: Maybe we could hide the internal map, and CatalogServiceCatalog could just have a method that would return the last event time for a (db, table, partition). http://gerrit.cloudera.org:8080/#/c/19484/3/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@2536 PS3, Line 2536: .containsKey(partitionMapKey) && : partitionMap.get(partitionMapKey) nit: just to avoid doing 2 lookups, we could just invoke a single get(), then check if the returned value was null. http://gerrit.cloudera.org:8080/#/c/19484/3/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/3/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@4576 PS3, Line 4576: int currentTimeinSecs = catalog_.now(); : Map<String, Integer> partitionMap = : catalog_.getPartitionMapFromRefreshMap(dbName, tblName); : for (String partName : partNames) { : partitionMap.put(partName, currentTimeinSecs); : } : catalog_.addToRefreshMetadataMap(dbName + "." + tblName, partitionMap); nit: Maybe CatalogServiceCatalog could have a method that would update its map, e.g.: updateEventTimes(db, tbl, listOfPartitions); http://gerrit.cloudera.org:8080/#/c/19484/3/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@6490 PS3, Line 6490: // Update the entries in the refresh metadata map : Integer currentTimeinSecs = catalog_.now(); : String dbName = tblName.getDb(); : String tableName = tblName.getTbl(); : if (req.isIs_refresh()) { : Map<String, Integer> partitionMap = catalog_.getPartitionMapFromRefreshMap( : dbName, tableName); : if (partVals == null) { // update table entry, since partitions are null : partitionMap.put(HdfsTable.DEFAULT_PARTITION_NAME, currentTimeinSecs); : } else { // update all the partitions values : for (String partitionName : partVals) { : partitionMap.put(partitionName, currentTimeinSecs); : } : } : catalog_.addToRefreshMetadataMap(dbName + "." + tableName, partitionMap); Thic could be also moved to a new 'updateEventTimes' method. -- 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-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Fri, 03 Mar 2023 14:04:15 +0000 Gerrit-HasComments: Yes
