Quanlong Huang 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 2: (3 comments) Thanks for fixing this quickly! Just have some minor comments. 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: msTbl_ = tbl.getMetaStoreTable(); Initializing this field here seems strange since this is a check method and is not expected to have side effects. I guess it's added here since this is the first method that uses this field. Maybe we need a Prepare or Finalize method to set fields before procesing the event. Could you add a TODO for this? 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@3659 PS2, Line 3659: public void testAllocWriteIdEventAfterLoadTable(String tblName, boolean isPartitioned) It seems we can consolidate this with testAllocWriteIdEvent() by adding a parameter of "boolean loadTable". The majority of the method codes are the same. http://gerrit.cloudera.org:8080/#/c/21087/2/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@3694 PS2, Line 3694: Can we add tests for ReloadEvent and CommitCompactionEvent? -- 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: 2 Gerrit-Owner: 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: Thu, 29 Feb 2024 11:09:56 +0000 Gerrit-HasComments: Yes
