Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/17308 )
Change subject: IMPALA-10502: Handle CREATE/DROP events correctly ...................................................................... Patch Set 5: (17 comments) The solution makes sense to me. I'm still looking into more detailed cases. Currently feel good :) http://gerrit.cloudera.org:8080/#/c/17308/5/fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java File fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java: http://gerrit.cloudera.org:8080/#/c/17308/5/fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java@a689 PS5, Line 689: Will removing these break third party extensions? If not, we can remove the above comment as well. http://gerrit.cloudera.org:8080/#/c/17308/5/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java File fe/src/main/java/org/apache/impala/catalog/HdfsTable.java: http://gerrit.cloudera.org:8080/#/c/17308/5/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@874 PS5, Line 874: public List<HdfsPartition> createAndLoadPartitions(IMetaStoreClient client, Can we refactor this with the above method? The major logics are the same. http://gerrit.cloudera.org:8080/#/c/17308/5/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@883 PS5, Line 883: msPartitionsToEventId : .containsKey(partName) nit: can fit into one line http://gerrit.cloudera.org:8080/#/c/17308/5/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@2436 PS5, Line 2436: for (int i=0; i<partitionColumns.size(); i++) nit: I think we prefer ++i and some spaces for (int i = 0; i < partitionColumns.size(); ++i) http://gerrit.cloudera.org:8080/#/c/17308/5/fe/src/main/java/org/apache/impala/catalog/TableLoader.java File fe/src/main/java/org/apache/impala/catalog/TableLoader.java: http://gerrit.cloudera.org:8080/#/c/17308/5/fe/src/main/java/org/apache/impala/catalog/TableLoader.java@78 PS5, Line 78: synchronized (metastoreAccessLock_) { hmm.. not related to this patch, I think we don't need this anymore. We have used getTable() in many other places without synchronization and don't see HIVE-5457 occurs anymore. Should we remove it BTW or do it in a separate JIRA? http://gerrit.cloudera.org:8080/#/c/17308/5/fe/src/main/java/org/apache/impala/catalog/TableLoader.java@112 PS5, Line 112: table.setCreateEventId(eventId); I'm not clear on the purpose of this. Do we depend on the createEventId when loading the table now? http://gerrit.cloudera.org:8080/#/c/17308/5/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/17308/5/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@787 PS5, Line 787: Reference<Boolean> tableAddedLater = new Reference<>(); nit: could you move this to line 805 since it's only used there? http://gerrit.cloudera.org:8080/#/c/17308/5/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@809 PS5, Line 809: before nit: "original" makes more sense for me :) http://gerrit.cloudera.org:8080/#/c/17308/5/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1305 PS5, Line 1305: private Table addHdfsPartitions(MetaStoreClient msClient, Table tbl, Can we refactor this with the above method? The major logics are the same. http://gerrit.cloudera.org:8080/#/c/17308/5/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1773 PS5, Line 1773: // this means that the database was recreated from some other application while : // this create database operation was in progress. We bail out by throwing an error : // in this case because it is possible the database which the user was trying to : // create was not the one which was eventually recreated in the metastore. : Preconditions.checkState(events.size() == 1, : "Database was recreated in metastore while " : + "createDatabase operation was in progress"); > yeah, it is slightly a tricky case to handle. Since metastore will only all Yeah, I think handling the IF NOT EXISTS case in this situation is a nice-to-have feature. Feel free to create a JIRA for it if it's complex. http://gerrit.cloudera.org:8080/#/c/17308/5/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3198 PS5, Line 3198: == > Not related to this patch, but this looks buggy. Could you change it to equ Thoughts? http://gerrit.cloudera.org:8080/#/c/17308/5/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3209 PS5, Line 3209: //TODO: Why don't we use the msTable above? > Good point. I think this is what we should have changed in this patch: http Thoughts? http://gerrit.cloudera.org:8080/#/c/17308/5/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@4015 PS5, Line 4015: * @param catName nit: can we remove this and the parameter in the method if we don't need it? http://gerrit.cloudera.org:8080/#/c/17308/5/fe/src/main/java/org/apache/impala/service/JniCatalog.java File fe/src/main/java/org/apache/impala/service/JniCatalog.java: http://gerrit.cloudera.org:8080/#/c/17308/5/fe/src/main/java/org/apache/impala/service/JniCatalog.java@185 PS5, Line 185: throw new CatalogException Throwing the execption will fail the process. Based on the aboved logs, it seems we just want to disable event processing. http://gerrit.cloudera.org:8080/#/c/17308/5/tests/custom_cluster/test_event_processing.py File tests/custom_cluster/test_event_processing.py: http://gerrit.cloudera.org:8080/#/c/17308/5/tests/custom_cluster/test_event_processing.py@285 PS5, Line 285: .format(db=db, i=1) No needed? It seems 'q' is already a concrete string. http://gerrit.cloudera.org:8080/#/c/17308/5/tests/custom_cluster/test_event_processing.py@286 PS5, Line 286: except Exception nit: might be helpful to print the exception as well http://gerrit.cloudera.org:8080/#/c/17308/5/tests/custom_cluster/test_event_processing.py@591 PS5, Line 591: implement it process it nit: process it -- To view, visit http://gerrit.cloudera.org:8080/17308 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia2c5e96b48abac015240f20295b3ec3b1d71f24a Gerrit-Change-Number: 17308 Gerrit-PatchSet: 5 Gerrit-Owner: Vihang Karajgaonkar <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Sourabh Goyal <[email protected]> Gerrit-Reviewer: Vihang Karajgaonkar <[email protected]> Gerrit-Comment-Date: Thu, 29 Apr 2021 12:38:12 +0000 Gerrit-HasComments: Yes
