Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/21029 )
Change subject: IMPALA-12543: Detect self-events before finishing DDL ...................................................................... Patch Set 10: (7 comments) http://gerrit.cloudera.org:8080/#/c/21029/10//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/21029/10//COMMIT_MSG@11 PS10, Line 11: happens I think that this is a bit misleading - the issue could probably happen on all build types, just it was witnessed on these ones, right? http://gerrit.cloudera.org:8080/#/c/21029/10//COMMIT_MSG@15 PS10, Line 15: locks is nit: lock is http://gerrit.cloudera.org:8080/#/c/21029/10/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/21029/10/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1078 PS10, Line 1078: public void cancelInflightEvent() { Can we call this when inflightEventRegistered_ is false? If not, then a precondition could be added for this. http://gerrit.cloudera.org:8080/#/c/21029/10/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1081 PS10, Line 1081: LOG.info("Cancel in-progress in-flight event of table " + table_.getFullName() : + ". isInsertEvent=" + isInsertEvent_ + " versionNumber=" + newVersionNumber_ : + " isRemoved=" + isRemoved); Can you add a comment about why isRemoved can be false? http://gerrit.cloudera.org:8080/#/c/21029/10/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1390 PS10, Line 1390: tbl.resetInProgressModification(); Shouldn't we do something with `modification` in case of an exception? Can we assume that modification.hasInProgressModification() is false at this point? http://gerrit.cloudera.org:8080/#/c/21029/10/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@6447 PS10, Line 6447: TException This won't catch exceptions from alterTableWithTransaction as it wraps the TException as ImpalaRuntimeException. The old try catch could be kept to around the non-transactional case to do this wrappin and the cancelInflightEvent could be called in an outer catch block http://gerrit.cloudera.org:8080/#/c/21029/10/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@6452 PS10, Line 6452: { This could be done outside the try block at line 6424 +nit: no braces needed -- To view, visit http://gerrit.cloudera.org:8080/21029 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8365c934349ad21a4d9327fc11594d2fc3445f79 Gerrit-Change-Number: 21029 Gerrit-PatchSet: 10 Gerrit-Owner: Riza Suminto <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Jason Fehr <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Riza Suminto <[email protected]> Gerrit-Reviewer: Sai Hemanth Gantasala <[email protected]> Gerrit-Comment-Date: Wed, 06 Mar 2024 16:02:51 +0000 Gerrit-HasComments: Yes
