Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/22807 )
Change subject: IMPALA-13989: Refresh table after rename ...................................................................... Patch Set 3: (6 comments) http://gerrit.cloudera.org:8080/#/c/22807/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/22807/2/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@5736 PS2, Line 5736: Pair<Table, Table> result = : catalog_.renameTable(tableName.toThrift(), newTableName.toThrift()); > We are holding the table lock of the old table here. Ideally the table lock I don't think holding versionLock_ throughout renameTable/invalidateTable helps. The main issue that affects us is when something happens between alter_table completing in HMS and running catalog_.renameTable. If something happens after renameTable, we have two cases - renameTable succeeded, so whatever happens after doesn't matter. - renameTable fails because something also happened before renameTable. We still have to handle the possibility that invalidateTable fails, because the table could have been renamed and then destination deleted before renameTable executes. A higher level lock could reduce the possibility of that occurring, but we would need IMPALA-13850 and some other work to be sure, and that seems outside the scope of this fix. http://gerrit.cloudera.org:8080/#/c/22807/2/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@5747 PS2, Line 5747: updated > nit: updated (successfully removed) Done http://gerrit.cloudera.org:8080/#/c/22807/2/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@5753 PS2, Line 5753: response.result.addToRemoved_catalog_objects(wantMinimalResult ? : oldTbl.toInvalidationObject() : oldTbl.toMinimalTCatalogObject()); > I don't think it matters, but I can do that. Done http://gerrit.cloudera.org:8080/#/c/22807/2/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@5763 PS2, Line 5763: tableName > Yes, that's what I was imagining. I didn't specifically reproduce that issu Done http://gerrit.cloudera.org:8080/#/c/22807/3/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/22807/3/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@5755 PS3, Line 5755: second > Nit: wouldn't something like 'newTblCatObject' easier to understand? Done http://gerrit.cloudera.org:8080/#/c/22807/3/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@5765 PS3, Line 5765: } else { > We could add a precondition check that 'result.first' is not NULL if 'resul Done -- To view, visit http://gerrit.cloudera.org:8080/22807 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic2a276b6e5ceb35b7f3ce788cc47052387ae8980 Gerrit-Change-Number: 22807 Gerrit-PatchSet: 3 Gerrit-Owner: Michael Smith <michael.sm...@cloudera.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Daniel Becker <daniel.bec...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Joe McDonnell <joemcdonn...@cloudera.com> Gerrit-Reviewer: Michael Smith <michael.sm...@cloudera.com> Gerrit-Reviewer: Quanlong Huang <huangquanl...@gmail.com> Gerrit-Reviewer: Riza Suminto <riza.sumi...@cloudera.com> Gerrit-Comment-Date: Thu, 24 Apr 2025 17:11:04 +0000 Gerrit-HasComments: Yes
