Quanlong Huang 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 'result.version' is sent to statestore." 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 non-existing partition, we don't need to wait since no updates will be sent" 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. 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 long. Maybe "waitForSyncDdl" is ok. 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 it as 'topicVersion' to be clear. 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 configurable as a startup flag and test it with a smaller value. 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? -- 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 08:25:54 +0000 Gerrit-HasComments: Yes
