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 8: (5 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: protected int getPort() throws CatalogException { > This is supposed to be only used during testing ? I don't recollect now why I had to make it public (maybe did this when adding new tests). Will fix it. 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: syncToLatestEventId(tbl, apiName); > There are so many TODOs in patch which needs discussion. I think you should Yes that makes sense. I had intentionally added so that I don't miss getting feedback on them. Will remove them before the merge into master branch 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: @Test > Looks like no changes in this file. Ack 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.setCatalogOpExecutor(opExecutor); > Please remove the statements, instead of commenting Left them as such because of some back and forth code changes. Will remove it. 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 ? Yes the flag is false by default. But these tests would start failing in future when the flag is turned on. And these tests are supposed to work with flag set to false. Therefore added a config for it here. -- 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: 8 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: Mon, 27 Sep 2021 17:39:55 +0000 Gerrit-HasComments: Yes
