Sourabh Goyal has posted comments on this change. ( http://gerrit.cloudera.org:8080/17859 )
Change subject: IMPALA-10926: Sync db/table in catalog cache to latest HMS event id when performing DDL operations via catalog HMS endpoints ...................................................................... Patch Set 6: (20 comments) http://gerrit.cloudera.org:8080/#/c/17859/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/17859/2/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@223 PS2, Line 223: //value of timeout for the topic update thread while waiting on the table lock. > 7200000 seems to be too large. How did you pick this value ? I didn't choose the value. It was already present. http://gerrit.cloudera.org:8080/#/c/17859/2/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@2142 PS2, Line 2142: // Return the table if it is already loaded or submit a new load request. > Why do you have many .trace() instead of .debug() ? VIhang had suggested to convert them to trace instead of debug so as to have lesser logs. http://gerrit.cloudera.org:8080/#/c/17859/2/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@3654 PS2, Line 3654: Preconditions.checkArgument(factory instanceof EventFactoryForSyncToLatestEvent, > Since its no longer final, can you check the code to make sure, we are not I have refactored the code in later patches to not set tableloading manager from outside. http://gerrit.cloudera.org:8080/#/c/17859/2/fe/src/main/java/org/apache/impala/catalog/TableLoader.java File fe/src/main/java/org/apache/impala/catalog/TableLoader.java: http://gerrit.cloudera.org:8080/#/c/17859/2/fe/src/main/java/org/apache/impala/catalog/TableLoader.java@51 PS2, Line 51: private final CatalogServiceCatalog catalog_; > What is the difference between LoggerFactory vs Logger ? LoggerFactory is from slf4j whereas previous logger was from log4j http://gerrit.cloudera.org:8080/#/c/17859/2/fe/src/main/java/org/apache/impala/catalog/TableLoader.java@164 PS2, Line 164: } > Can there be multiple threads trying to do full table reload at the same ti We create a new table object at line 138 so it is thread safe and no lock is required. http://gerrit.cloudera.org:8080/#/c/17859/2/fe/src/main/java/org/apache/impala/catalog/TableLoadingMgr.java File fe/src/main/java/org/apache/impala/catalog/TableLoadingMgr.java: http://gerrit.cloudera.org:8080/#/c/17859/2/fe/src/main/java/org/apache/impala/catalog/TableLoadingMgr.java@159 PS2, Line 159: private final CatalogServiceCatalog catalog_; > Why are you removing "final" for CatalogServiceCatalog and CatalogOpExecuto I have fixed it in subsequent patch sets. http://gerrit.cloudera.org:8080/#/c/17859/2/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/17859/2/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@266 PS2, Line 266: } > I think it all depends on whether we can toggle this flag, without restarti I tried reusing the existing event factory and caught few issues in the test suite. For now, we are keeping event factory for syncing to latest event id separate. http://gerrit.cloudera.org:8080/#/c/17859/2/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@835 PS2, Line 835: * Skip this event if the table is already synced till this event id. Otherwise > Please write the Javadoc for this method. Sure. Thanks for catching it. http://gerrit.cloudera.org:8080/#/c/17859/2/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@966 PS2, Line 966: && dbName_.equalsIgnoreCase(alterTableEvent.msTbl_.getDbName()) > May be you can add default interface method which returns false in the abst The parent class i.e MetastoreTableEvent has a default implementation which works for all table events except create and drop event. http://gerrit.cloudera.org:8080/#/c/17859/2/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@1163 PS2, Line 1163: > Please use the variables directly from tableBefore and tableAfter. Sure. http://gerrit.cloudera.org:8080/#/c/17859/2/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@1504 PS2, Line 1504: > You can avoid most of these by having default implementation, which returns Have fixed it for DatabaseEvent classes http://gerrit.cloudera.org:8080/#/c/17859/2/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java File fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java: http://gerrit.cloudera.org:8080/#/c/17859/2/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java@357 PS2, Line 357: from cache and subsequent create_table event would add > Please remove these TODOs or update them as per what is needed in future, b Yes, I will remove/update them after the discussion. http://gerrit.cloudera.org:8080/#/c/17859/2/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java@475 PS2, Line 475: return db.getName().equalsIgnoreCase(event.getDbName()); > I think it should become a utility method. Otherwise, everywhere we have to Didn't understand. The method is static. In a way, it is already a utility method. http://gerrit.cloudera.org:8080/#/c/17859/2/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java@899 PS2, Line 899: return metrics_; > Why this has to be changed to "protected", if its only for testing ? Can't recollect my thoughts at that time. I am overriding this method in SynchronousHMSEventProcessorForTests for testing purpose. Removing this change. http://gerrit.cloudera.org:8080/#/c/17859/2/fe/src/main/java/org/apache/impala/catalog/metastore/CatalogMetastoreServiceHandler.java File fe/src/main/java/org/apache/impala/catalog/metastore/CatalogMetastoreServiceHandler.java: http://gerrit.cloudera.org:8080/#/c/17859/2/fe/src/main/java/org/apache/impala/catalog/metastore/CatalogMetastoreServiceHandler.java@110 PS2, Line 110: > Please remove the variable, if you don't need it. Ack http://gerrit.cloudera.org:8080/#/c/17859/2/fe/src/main/java/org/apache/impala/catalog/metastore/CatalogMetastoreServiceHandler.java@117 PS2, Line 117: public GetTableResult get_table_req(GetTableRequest getTableRequest) > I don't think that would be a good idea. I too had the same thought, just wanted to confirm. Thanks for the confirmation. http://gerrit.cloudera.org:8080/#/c/17859/2/fe/src/main/java/org/apache/impala/catalog/metastore/CatalogMetastoreServiceHandler.java@509 PS2, Line 509: syncToLatestEventId(tbl, apiName); > Hope you are using the Java template. Yes I am http://gerrit.cloudera.org:8080/#/c/17859/2/fe/src/main/java/org/apache/impala/catalog/metastore/CatalogMetastoreServiceHandler.java@607 PS2, Line 607: > There is no harm in doing it. Ack http://gerrit.cloudera.org:8080/#/c/17859/2/fe/src/main/java/org/apache/impala/catalog/metastore/CatalogMetastoreServiceHandler.java@1332 PS2, Line 1332: T resp = task.execute(); > Do you have to distinguish between database rename and table rename ? Didn't get you. We consider a table as renamed if It is moved to a different db or it is in same db but name is changed. http://gerrit.cloudera.org:8080/#/c/17859/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/17859/2/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@749 PS2, Line 749: if (existingTable != null) { @Vihang: I have modified the check to also check for create event id in an existing table. Please take a look. -- To view, visit http://gerrit.cloudera.org:8080/17859 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I36364e401911352c4666674eb98c8d61bbaae9b9 Gerrit-Change-Number: 17859 Gerrit-PatchSet: 6 Gerrit-Owner: Sourabh Goyal <soura...@cloudera.com> Gerrit-Reviewer: Anonymous Coward <kis...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Sourabh Goyal <soura...@cloudera.com> Gerrit-Comment-Date: Thu, 23 Sep 2021 22:09:06 +0000 Gerrit-HasComments: Yes
