Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/13904 )
Change subject: IMPALA-8434: retain tables and functions in altering database ...................................................................... Patch Set 2: (3 comments) http://gerrit.cloudera.org:8080/#/c/13904/2/fe/src/main/java/org/apache/impala/catalog/ImpaladCatalog.java File fe/src/main/java/org/apache/impala/catalog/ImpaladCatalog.java: http://gerrit.cloudera.org:8080/#/c/13904/2/fe/src/main/java/org/apache/impala/catalog/ImpaladCatalog.java@392 PS2, Line 392: addDb(newDb); Thanks to Vihang's comment in person. Will move this below after adding existing tables and functions to avoid race of seeing the empty new database. http://gerrit.cloudera.org:8080/#/c/13904/2/fe/src/main/java/org/apache/impala/catalog/ImpaladCatalog.java@394 PS2, Line 394: CatalogObjectVersionSet.INSTANCE.updateVersions( : existingDb.getCatalogVersion(), catalogVersion); : CatalogObjectVersionSet.INSTANCE.removeAll(existingDb.getTables()); : CatalogObjectVersionSet.INSTANCE.removeAll( : existingDb.getFunctions(null, new PatternMatcher())); : // IMPALA-8434: add back the existing tables/functions. Note that their version : // counters in CatalogObjectVersionSet have been decreased by the above removeAll : // statements, meaning their references from the old db are deleted since the old : // db object has been replaced by newDb. : for (Table tbl: existingDb.getTables()) { : newDb.addTable(tbl); : } > Oops I only went one level deep and checked tableCache_.add(table); Didn't Never mind :) http://gerrit.cloudera.org:8080/#/c/13904/2/tests/metadata/test_ddl.py File tests/metadata/test_ddl.py: http://gerrit.cloudera.org:8080/#/c/13904/2/tests/metadata/test_ddl.py@241 PS2, Line 241: self.client.execute("create table {0}.tbl (i int)".format(unique_database)) > nit: how about merging this with the above test? I think they are testing different points. The above one is for the functionality of altering owner for the database. This one is for checking metadata according to the bug. It'd be better to let them act as unit tests. So if someone fails them in the future, it'll be easier to know exactly what happens. -- To view, visit http://gerrit.cloudera.org:8080/13904 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia3dc9857fd2733e20cf10fbe17bb1a4670d7d015 Gerrit-Change-Number: 13904 Gerrit-PatchSet: 2 Gerrit-Owner: Quanlong Huang <[email protected]> Gerrit-Reviewer: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Vihang Karajgaonkar <[email protected]> Gerrit-Comment-Date: Thu, 25 Jul 2019 18:50:58 +0000 Gerrit-HasComments: Yes
