Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/21029 )
Change subject: IMPALA-12543: Detect self-events before finishing DDL ...................................................................... Patch Set 12: (10 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: > I think that this is a bit misleading - the issue could probably happen on I think so. Removed this sentence. http://gerrit.cloudera.org:8080/#/c/21029/10//COMMIT_MSG@15 PS10, Line 15: part1). > nit: lock is Done http://gerrit.cloudera.org:8080/#/c/21029/7/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/7/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@4493 PS7, Line 4493: partitionCachingOpMap); > Probably not, but I prefer matching the old-behavior like in old-code, L114 Added check and log at patch set 11. http://gerrit.cloudera.org:8080/#/c/21029/7/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@6135 PS7, Line 6135: if (cachePoolName != null) { > This is one spot where I'm no sure about. What if some alterPartitions went I'm guessing that partition-level event is simpler, such that registerInflightEvent() is only needed if no exception is thrown at all. Added comment. http://gerrit.cloudera.org:8080/#/c/21029/9/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/9/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@5317 PS9, Line 5317: * If purge is true, partition data is > I'll try run core tests with this removed. Exhaustive tests pass without this catalog version bumped. Removed this line. 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: */ > Can we call this when inflightEventRegistered_ is false? If not, then a pre Done http://gerrit.cloudera.org:8080/#/c/21029/10/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1081 PS10, Line 1081: // Since inflightEventRegistered_ is True, isRemoved should also be True. : // Unless, if the in-flight event has been concurrently removed by other call site : // such as MetastoreEvent.isSelfE > Can you add a comment about why isRemoved can be false? Done http://gerrit.cloudera.org:8080/#/c/21029/10/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1390 PS10, Line 1390: // Make sure all the modifications are done. > Shouldn't we do something with `modification` in case of an exception? Can I just add cancelInflightEvent() here. Not sure how to undo table_.hasInProgressModification(). http://gerrit.cloudera.org:8080/#/c/21029/10/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@6447 PS10, Line 6447: computing/ > This won't catch exceptions from alterTableWithTransaction as it wraps the Done 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 Done -- 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: 12 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 18:59:21 +0000 Gerrit-HasComments: Yes
