Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/21143 )
Change subject: IMPALA-12856: Event processor should ignore processing partition with empty partition values ...................................................................... Patch Set 2: (6 comments) http://gerrit.cloudera.org:8080/#/c/21143/1/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/21143/1/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@2846 PS1, Line 2846: public int reloadPartitionsFromNames(long eventId, String eventType, > Ack Shouldn't the 'reason' already include the event type? I think that ideally 'reason' would also contain the eventId, so no additional argument would have to passed just for the sake of logging, but this seems like a larger change. http://gerrit.cloudera.org:8080/#/c/21143/2/fe/src/main/java/org/apache/impala/util/DebugUtils.java File fe/src/main/java/org/apache/impala/util/DebugUtils.java: http://gerrit.cloudera.org:8080/#/c/21143/2/fe/src/main/java/org/apache/impala/util/DebugUtils.java@77 PS2, Line 77: // debug action label for mock that metastore returns partitions with empty values Can you add a comment about the Jira to explain why this error injection is added? http://gerrit.cloudera.org:8080/#/c/21143/2/fe/src/main/java/org/apache/impala/util/MetaStoreUtil.java File fe/src/main/java/org/apache/impala/util/MetaStoreUtil.java: http://gerrit.cloudera.org:8080/#/c/21143/2/fe/src/main/java/org/apache/impala/util/MetaStoreUtil.java@212 PS2, Line 212: if (DebugUtils.hasDebugAction(BackendConfig.INSTANCE.debugActions(), Can you add a comment about the Jira to explain why this error injection is added? http://gerrit.cloudera.org:8080/#/c/21143/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/21143/2/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@3757 PS2, Line 3757: testEmptyPartitionValues Can you add a comment about being the regression test for the Jira? http://gerrit.cloudera.org:8080/#/c/21143/2/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@3837 PS2, Line 3837: BackendConfig.INSTANCE.setDebugActions(DebugUtils.MOCK_EMPTY_PARTITION_VALUES); Is this needed here? Doesn't have an affect only when Impala processes the event or reloads the table? It also doesn't seem intuitive that this function has the side effect of changing the backed flags. http://gerrit.cloudera.org:8080/#/c/21143/2/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@3847 PS2, Line 3847: BackendConfig.INSTANCE.setDebugActions(prevFlag); It doesn't seem intuitive to me that we reset this here. -- To view, visit http://gerrit.cloudera.org:8080/21143 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id2469930ccd74948325f1723bd8b2bd6aad02d09 Gerrit-Change-Number: 21143 Gerrit-PatchSet: 2 Gerrit-Owner: Sai Hemanth Gantasala <[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: Thu, 14 Mar 2024 11:10:04 +0000 Gerrit-HasComments: Yes
