ttttttz has posted comments on this change. ( http://gerrit.cloudera.org:8080/20490 )
Change subject: IMPALA-12448: Avoid getting stuck when refreshing a non-existent partition ...................................................................... Patch Set 7: (7 comments) http://gerrit.cloudera.org:8080/#/c/20490/7/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/20490/7/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@6583 PS7, Line 6583: // If sync ddl is true, we should wait for the version specified in > nit: "If sync_ddl is true, we should wait until the version specified in 'r Ack http://gerrit.cloudera.org:8080/#/c/20490/7/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@6584 PS7, Line 6584: in some exceptional cases, : // we shouldn't wait because the table version has not changed after : // resetting metadata, such as refreshing a non-existent partition. > nit: "in some corner cases where no metadata is changed, e.g. refreshing a Ack http://gerrit.cloudera.org:8080/#/c/20490/7/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@6587 PS7, Line 6587: // If not, the query may be stuck for a long time. > nit: this sentense is too verbose. We can remove it. Ack http://gerrit.cloudera.org:8080/#/c/20490/7/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@6588 PS7, Line 6588: needWaitSyncDdlVersion > nit: "needWaitForSyncDdlVersion" sounds more correct in syntax but it's too Ack http://gerrit.cloudera.org:8080/#/c/20490/7/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@6730 PS7, Line 6730: syncDdlVersion > nit: it's a version of the catalog topic, not a catalog version. Let's name Ack http://gerrit.cloudera.org:8080/#/c/20490/7/tests/metadata/test_refresh_partition.py File tests/metadata/test_refresh_partition.py: http://gerrit.cloudera.org:8080/#/c/20490/7/tests/metadata/test_refresh_partition.py@199 PS7, Line 199: i = 2000 > This runs pretty long. We can make TOPIC_UPDATE_LOG_GC_FREQUENCY configurab Ack http://gerrit.cloudera.org:8080/#/c/20490/7/tests/metadata/test_refresh_partition.py@202 PS7, Line 202: sleep(2) > What's the purpose for the sleep here? Ensure that updates are sent to the statestored after each refresh. -- To view, visit http://gerrit.cloudera.org:8080/20490 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iace7cdadda300b03896f92415822266354421887 Gerrit-Change-Number: 20490 Gerrit-PatchSet: 7 Gerrit-Owner: ttttttz <2433038...@qq.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Quanlong Huang <huangquanl...@gmail.com> Gerrit-Reviewer: ttttttz <2433038...@qq.com> Gerrit-Comment-Date: Thu, 02 Nov 2023 15:52:07 +0000 Gerrit-HasComments: Yes
