Vihang Karajgaonkar has posted comments on this change. ( http://gerrit.cloudera.org:8080/14307 )
Change subject: IMPALA-7506: support global INVALIDATE METADATA in local catalog mode ...................................................................... Patch Set 8: (5 comments) The approach looks good to me. Took another pass at this and left some suggestions below. Thanks! http://gerrit.cloudera.org:8080/#/c/14307/8/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/14307/8/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@604 PS8, Line 604: lastResetStartVersion nit, I think its better to make this variable as final (same for fromVersion, toVersion and nativeCatalogServerPtr) http://gerrit.cloudera.org:8080/#/c/14307/8/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@763 PS8, Line 763: catalog.setCatalog(new TCatalog(catalogServiceId_, ctx.lastResetStartVersion)); Just a thought to further optimize this. If we know that the lastResetStartVersion has changed here and if we are using local catalog mode, do we need to send all the dbs, tables, etc? Can we just send a light weight topic update which only has a TCatalogObject? It seems like on the coordinator side, all we really care of is the min catalog version, current catalog version which is both available in the TCatalogObject type. Do you see any problems with this? http://gerrit.cloudera.org:8080/#/c/14307/8/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1135 PS8, Line 1135: catalogTbl.setTable(new TTable(tbl.db_.getName(), tbl.name_)); : catalogTbl.setCatalog_version(tbl.getCatalogVersion()); I think its really good that we are not serializing the whole table object here. However, I am not sure if can do this without taking the tbl lock. For instance, we are getting the tbl version couple of times here. It is possible that the version changes between line 1128 and 1136. Can the table name change here too from another thread? http://gerrit.cloudera.org:8080/#/c/14307/8/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1138 PS8, Line 1138: return Do we need to add this to topicUpdateEntry before returning? Otherwise, any other DDL with sync_ddl turned on will not be able to find a covering topic for this object. right? http://gerrit.cloudera.org:8080/#/c/14307/8/tests/authorization/test_ranger.py File tests/authorization/test_ranger.py: http://gerrit.cloudera.org:8080/#/c/14307/8/tests/authorization/test_ranger.py@a60 PS8, Line 60: so we don't need --use_local_catalog passed to catalogd anymore? -- To view, visit http://gerrit.cloudera.org:8080/14307 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib61a7ab1ffa062620ffbc2dadc34bd7a8ca9e549 Gerrit-Change-Number: 14307 Gerrit-PatchSet: 8 Gerrit-Owner: Quanlong Huang <huangquanl...@gmail.com> Gerrit-Reviewer: Bharath Vissapragada <bhara...@apache.org> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Quanlong Huang <huangquanl...@gmail.com> Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Reviewer: Vihang Karajgaonkar <vih...@cloudera.com> Gerrit-Comment-Date: Mon, 28 Oct 2019 23:58:21 +0000 Gerrit-HasComments: Yes
