Sourabh Goyal has posted comments on this change. ( http://gerrit.cloudera.org:8080/17576 )
Change subject: IMPALA-10746: Drop table/db from catalog cache when drop table/db HMS apis are accessed from catalog's metastore server. ...................................................................... Patch Set 13: (4 comments) http://gerrit.cloudera.org:8080/#/c/17576/10/fe/src/main/java/org/apache/impala/catalog/metastore/MetastoreServiceHandler.java File fe/src/main/java/org/apache/impala/catalog/metastore/MetastoreServiceHandler.java: http://gerrit.cloudera.org:8080/#/c/17576/10/fe/src/main/java/org/apache/impala/catalog/metastore/MetastoreServiceHandler.java@462 PS10, Line 462: : // Parse db name. Throw error if parsing fails. : String dbName; : String catName; : String[] catAndDbName = : > I agree with the point 2 above. But I think the deleteEventLog should be ad Sure. http://gerrit.cloudera.org:8080/#/c/17576/13/fe/src/main/java/org/apache/impala/catalog/metastore/MetastoreServiceHandler.java File fe/src/main/java/org/apache/impala/catalog/metastore/MetastoreServiceHandler.java: http://gerrit.cloudera.org:8080/#/c/17576/13/fe/src/main/java/org/apache/impala/catalog/metastore/MetastoreServiceHandler.java@479 PS13, Line 479: .getNextMetastoreEvents(catalog_, currentEventId, > I believe it fetches for DROP_TABLE events as well because a cascade drop o You are right. Cascade for drop_database is supported (and implemented) in HiveMetastoreClient but it is not part of thrift interface. We don't need to check for drop_table events here. http://gerrit.cloudera.org:8080/#/c/17576/13/fe/src/main/java/org/apache/impala/catalog/metastore/MetastoreServiceHandler.java@456 PS13, Line 456: if (!invalidateCacheOnDDLs_) { : LOG.debug("invalidateCacheOnDDLs_ flag is false, skipping " + : "cache update for hms operation drop_database on db: " + : databaseName); : return; : } : : // Parse db name. Throw error if parsing fails. : String dbName; : String catName; : String[] catAndDbName = : MetaStoreUtils.parseDbName(databaseName, serverConf_); : catName = catAndDbName[0]; : dbName = catAndDbName[1]; : // get DB to drop from catalog cache : Db dbToDrop = catalog_.getDb(dbName); : if(dbToDrop == null) { : // dbToDrop doesn't exist in cache : return; : } : List<NotificationEvent> events; : try { : events = MetastoreEventsProcessor : .getNextMetastoreEvents(catalog_, currentEventId, : event -> event.getEventType() : .equalsIgnoreCase(MetastoreEvents.DropDatabaseEvent : .DROP_DATABASE_EVENT_TYPE) : && catName.equalsIgnoreCase(event.getCatName()) : && dbName.equalsIgnoreCase(event.getDbName())); : } catch (ImpalaException e) { : String errorMsg = "Unable to process Drop database event for db: " + : dbToDrop.getName(); : LOG.error(errorMsg, e); : throw new MetaException(errorMsg); : } : if (events.isEmpty()) { : throw new MetaException( : "Drop database event not received. Check if notification " + : "events are configured in hive metastore"); : } : long dropEventId = events.get(events.size() - 1).getEventId(); : boolean isRemoved = : catalogOpExecutor_.removeDbIfNotAddedLater(dropEventId, : dbToDrop.getMetaStoreDb()); : if (isRemoved) { : LOG.info("Removed database: " + databaseName + : " from cache due to HMS API: drop_database"); > I felt the code readability improves if you refactor it out. But I don't ha Sure. I will move it to a different method. http://gerrit.cloudera.org:8080/#/c/17576/13/fe/src/main/java/org/apache/impala/catalog/metastore/MetastoreServiceHandler.java@2968 PS13, Line 2968: Map<String, String> tblProperties = catalogTbl.getMetaStoreTable().getParameters(); : if (tblProperties == null || MetaStoreUtils.isTransactionalTable(tblProperties)) { > I agree that functionally they both for same. My point was more related to Got it. Will switch to using AcidUtils -- To view, visit http://gerrit.cloudera.org:8080/17576 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic2e2ad2630e2028b8ad26a6272ee766b27e0935c Gerrit-Change-Number: 17576 Gerrit-PatchSet: 13 Gerrit-Owner: Sourabh Goyal <[email protected]> Gerrit-Reviewer: Anonymous Coward <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Sourabh Goyal <[email protected]> Gerrit-Reviewer: Vihang Karajgaonkar <[email protected]> Gerrit-Reviewer: Yu-Wen Lai <[email protected]> Gerrit-Comment-Date: Tue, 03 Aug 2021 10:56:14 +0000 Gerrit-HasComments: Yes
