[email protected] 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 4: (6 comments) http://gerrit.cloudera.org:8080/#/c/17859/4/fe/src/main/java/org/apache/impala/catalog/metastore/CatalogMetastoreServer.java File fe/src/main/java/org/apache/impala/catalog/metastore/CatalogMetastoreServer.java: http://gerrit.cloudera.org:8080/#/c/17859/4/fe/src/main/java/org/apache/impala/catalog/metastore/CatalogMetastoreServer.java@253 PS4, Line 253: public int getPort() throws CatalogException { This is supposed to be only used during testing ? Why it has to be made public ? http://gerrit.cloudera.org:8080/#/c/17859/4/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/4/fe/src/main/java/org/apache/impala/catalog/metastore/CatalogMetastoreServiceHandler.java@609 PS4, Line 609: // TODO: Should we sync the table till latest event id if partitions list There are so many TODOs in patch which needs discussion. I think you should just discuss these internally and finalize the implementation and remove these TODOs, if they are not about some missing logic that needs to be implemented in future. http://gerrit.cloudera.org:8080/#/c/17859/4/fe/src/test/java/org/apache/impala/catalog/metastore/CatalogHmsFileMetadataTest.java File fe/src/test/java/org/apache/impala/catalog/metastore/CatalogHmsFileMetadataTest.java: http://gerrit.cloudera.org:8080/#/c/17859/4/fe/src/test/java/org/apache/impala/catalog/metastore/CatalogHmsFileMetadataTest.java@54 PS4, Line 54: Looks like no changes in this file. http://gerrit.cloudera.org:8080/#/c/17859/4/fe/src/test/java/org/apache/impala/catalog/metastore/CatalogHmsSyncToLatestEventIdTest.java File fe/src/test/java/org/apache/impala/catalog/metastore/CatalogHmsSyncToLatestEventIdTest.java: http://gerrit.cloudera.org:8080/#/c/17859/4/fe/src/test/java/org/apache/impala/catalog/metastore/CatalogHmsSyncToLatestEventIdTest.java@174 PS4, Line 174: public void testAlterDatabase() throws Exception { How do make sure future DDLs will sync to latest event ID ? http://gerrit.cloudera.org:8080/#/c/17859/4/fe/src/test/java/org/apache/impala/testutil/CatalogServiceTestCatalog.java File fe/src/test/java/org/apache/impala/testutil/CatalogServiceTestCatalog.java: http://gerrit.cloudera.org:8080/#/c/17859/4/fe/src/test/java/org/apache/impala/testutil/CatalogServiceTestCatalog.java@118 PS4, Line 118: //cs.setTableLoadingMgr(new TableLoadingMgr(opExecutor, 16)); Please remove the statements, instead of commenting http://gerrit.cloudera.org:8080/#/c/17859/4/tests/custom_cluster/test_metastore_service.py File tests/custom_cluster/test_metastore_service.py: http://gerrit.cloudera.org:8080/#/c/17859/4/tests/custom_cluster/test_metastore_service.py@126 PS4, Line 126: "--enable_sync_to_latest_event_on_ddls=false" Will it not be false by default ? Do we need these changes ? -- 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: 4 Gerrit-Owner: Sourabh Goyal <[email protected]> Gerrit-Reviewer: Anonymous Coward <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Comment-Date: Thu, 23 Sep 2021 17:10:21 +0000 Gerrit-HasComments: Yes
