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 4: (7 comments) 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 Integer getEventTimeFromRefreshMap(String dbName, String tblName, : String partitionName) { : > Seems unused, in which case there is no need for the AtomicReference. Ack 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 Ack 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 Ack 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: Integer eventTime = catalog_.getEventTimeFromRefreshMap(dbName_, tblName_, : partitionMapKey); > nit: Maybe we could hide the internal map, and CatalogServiceCatalog could Ack http://gerrit.cloudera.org:8080/#/c/19484/3/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@2536 PS3, Line 2536: null && eventTime >= eventTime_) { : return true; > nit: just to avoid doing 2 lookups, we could just invoke a single get(), th Ack 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: catalog_.updateEventTimesOfRefreshMap(dbName, tblName, partNames); : return numOfPartsReloaded; : } catch (TableLoadingException e) { : LOG.info("Could not reload {} partitions of table {}", partNames.size(), : table.getFullName(), e); : } catch (InternalException e) { : errorOccured = true; > nit: Maybe CatalogServiceCatalog could have a method that would update its Ack http://gerrit.cloudera.org:8080/#/c/19484/3/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@6490 PS3, Line 6490: // remove the entries of table and its partitions : catalog_.removeEntryFromRefreshMap(dbName + "." + tableName); : } : // Get new catalog version for table refresh/invalidate. : long newCatalogVersion = updatedThriftTable.getCatalog_version(); : Map<String, String> tableParams = new HashMap<>(); : tableParams.put(MetastoreEventPropertyKey.CATALOG_SERVICE_ID.getKey(), : catalog_.getCatalogServiceId()); : tableParams.put(MetastoreEventPropertyKey.CATALOG_VERSION.getKey(), : String.valueOf(newCatalogVersion)); : MetastoreShim.fireReloadEventHelper(catalog_.getMetaStoreClient(), : req.isIs_refresh(), partVals, dbName, tableName, : tableParams); : if (req.isIs_refresh()) { : if (catalog_.tryLock(tbl, true, 600000)) { > Thic could be also moved to a new 'updateEventTimes' method. 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: 4 Gerrit-Owner: Sai Hemanth Gantasala <saihema...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Quanlong Huang <huangquanl...@gmail.com> Gerrit-Reviewer: Sai Hemanth Gantasala <saihema...@cloudera.com> Gerrit-Reviewer: Zoltan Borok-Nagy <borokna...@cloudera.com> Gerrit-Comment-Date: Fri, 03 Mar 2023 18:27:36 +0000 Gerrit-HasComments: Yes
