[email protected] has posted comments on this change. ( http://gerrit.cloudera.org:8080/21087 )
Change subject: IMPALA-12851: Fix AllocWriteIdEvent process issue to add txnId-tableWriteIds mapping ...................................................................... Patch Set 6: (6 comments) Reworked http://gerrit.cloudera.org:8080/#/c/21087/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/21087/4//COMMIT_MSG@11 PS4, Line 11: 1 > Isn't 1 a sub case of 2? After getting a CREATE_TABLE the table shouldn't b In case-1, catalog table is not present because CREATE_TABLE event is not processed by the time of AllocWriteIdEvent construction. In case-2, catalog table is present but it is not loaded. Have updated the description now. http://gerrit.cloudera.org:8080/#/c/21087/4/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java File fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java: http://gerrit.cloudera.org:8080/#/c/21087/4/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@3937 PS4, Line 3937: if (tbl.getNumClusteringCols() == 0) { > Can you add a comment with some explanation (or some pointer to an explanat Done http://gerrit.cloudera.org:8080/#/c/21087/4/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@3938 PS4, Line 3938: // For non-partitioned tables, we just reload the whole table without keeping > nit: missing "a"? Done http://gerrit.cloudera.org:8080/#/c/21087/2/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java File fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java: http://gerrit.cloudera.org:8080/#/c/21087/2/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@2824 PS2, Line 2824: // can be useful for other such events. > msTbl_ is used inside super.isEventProcessingDisabled() below. Added a TODO to have an init method to set fields that cannot be initialized in the event constructors and invoke it as the first step before processing event. http://gerrit.cloudera.org:8080/#/c/21087/4/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java File fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java: http://gerrit.cloudera.org:8080/#/c/21087/4/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@2824 PS4, Line 2824: can b > oops, I see that https://gerrit.cloudera.org/#/c/21087/2/fe/src/main/java/o Yes. Have added a TODO for now. This gerrit is required for https://gerrit.cloudera.org/#/c/21065/. Shall take it up later again as improvement. http://gerrit.cloudera.org:8080/#/c/21087/2/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java File fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java: http://gerrit.cloudera.org:8080/#/c/21087/2/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@3694 PS2, Line 3694: "alter table " + TEST_DB_NAME + '.' + tblName + " compact 'minor' and wait"); > I might miss something. Do we already have tests in test_events_custom_conf Added FE tests for both ReloadEvent and CommitCompactionEvent now -- To view, visit http://gerrit.cloudera.org:8080/21087 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8b1a918befd4ee694880fd4e3cc04cb55b64955f Gerrit-Change-Number: 21087 Gerrit-PatchSet: 6 Gerrit-Owner: Anonymous Coward <[email protected]> Gerrit-Reviewer: Anonymous Coward <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Sai Hemanth Gantasala <[email protected]> Gerrit-Comment-Date: Fri, 01 Mar 2024 16:54:29 +0000 Gerrit-HasComments: Yes
