Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/17253 )
Change subject: IMPALA-6671: Change wait for sync ddl timeout ...................................................................... Patch Set 4: (5 comments) Thanks for this work! The patch makes sense to me. http://gerrit.cloudera.org:8080/#/c/17253/4/be/src/catalog/catalog-server.cc File be/src/catalog/catalog-server.cc: http://gerrit.cloudera.org:8080/#/c/17253/4/be/src/catalog/catalog-server.cc@92 PS4, Line 92: DEFINE_int32(max_wait_time_for_sync_ddl_s, 0, "Maximum time (in seconds) until " Do you have a recommendation on what this should be when topic_update_tbl_max_wait_time_ms is enabled? I think it should be larger than the topic update frequency since we at least will wait for that time. http://gerrit.cloudera.org:8080/#/c/17253/4/be/src/catalog/catalog-server.cc@94 PS4, Line 94: will nit: will wait? http://gerrit.cloudera.org:8080/#/c/17253/4/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java File fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java: http://gerrit.cloudera.org:8080/#/c/17253/4/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@3094 PS4, Line 3094: long maxNumAttempts = 5; I think another solution is increasing maxNumAttempts with maxSkippedUpdatesLockContention_, then maybe we don't need introducing a new timeout flag which may be hard to tune. The rationale is that we will at most skip the table maxSkippedUpdatesLockContention_ times comparing to the legacy behavior. What do you think? http://gerrit.cloudera.org:8080/#/c/17253/4/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@3160 PS4, Line 3160: It checks nit: remove this? http://gerrit.cloudera.org:8080/#/c/17253/4/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@3168 PS4, Line 3168: timeoutSecs == 0 I think we should return false for negative values as well. -- To view, visit http://gerrit.cloudera.org:8080/17253 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I79e64cdec0e6aa7b597a47851b4b5c5441ca5528 Gerrit-Change-Number: 17253 Gerrit-PatchSet: 4 Gerrit-Owner: Vihang Karajgaonkar <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Comment-Date: Fri, 02 Apr 2021 02:14:29 +0000 Gerrit-HasComments: Yes
