Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/21430 )
Change subject: IMPALA-12680: Fix NullPointerException during AlterTableAddPartitions ...................................................................... Patch Set 3: (4 comments) http://gerrit.cloudera.org:8080/#/c/21430/1/fe/src/main/java/org/apache/impala/service/BackendConfig.java File fe/src/main/java/org/apache/impala/service/BackendConfig.java: http://gerrit.cloudera.org:8080/#/c/21430/1/fe/src/main/java/org/apache/impala/service/BackendConfig.java@463 PS1, Line 463: : public String debugActions() { return backendCfg_.debug_actions; } : > Oops! I forgot to remove this as this is not being used. Done http://gerrit.cloudera.org:8080/#/c/21430/3/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/21430/3/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1731 PS3, Line 1731: @Nullable Map<String, Long> partitionToEventId Can @Nullable annotation here dropped as well? Please also double check similar annotation for partitionToEventId parameters scattered at HdfsTable.java. I'm worried if they are indirectly used as boolean condition to take or skip certain code path. http://gerrit.cloudera.org:8080/#/c/21430/3/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/21430/3/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@4071 PS3, Line 4071: alterTableAddPartition(TEST_DB_NAME, testTable, partitionDef, : "enable_event_processor"); Since "enable_event_processor" restart EP, it will be great if we can also assert that EP see the self-events (from new partition add operation) coming back from HMS. http://gerrit.cloudera.org:8080/#/c/21430/3/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@4076 PS3, Line 4076: eventsProcessor_.getStatus() != EventProcessorStatus.ACTIVE Should this be a test assertion instead? I expect EP should be in ACTIVE status after debug action "enable_event_processor" invoked. If it is not ACTIVE after debug action invoked, then it is also not guaranteed that L4077 can actually bring EP to ACTIVE status again? -- To view, visit http://gerrit.cloudera.org:8080/21430 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I730fed311ebc09762dccc152d9583d5394b0b9b3 Gerrit-Change-Number: 21430 Gerrit-PatchSet: 3 Gerrit-Owner: Sai Hemanth Gantasala <saihema...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Michael Smith <michael.sm...@cloudera.com> Gerrit-Reviewer: Quanlong Huang <huangquanl...@gmail.com> Gerrit-Reviewer: Riza Suminto <riza.sumi...@cloudera.com> Gerrit-Reviewer: Sai Hemanth Gantasala <saihema...@cloudera.com> Gerrit-Comment-Date: Tue, 28 May 2024 23:57:03 +0000 Gerrit-HasComments: Yes
