Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/17645 )
Change subject: IMPALA-5476: Fix Catalogd restart bring about metadata is out of sync ...................................................................... Patch Set 10: (8 comments) The patch is pretty good now. Just have some minor comments. http://gerrit.cloudera.org:8080/#/c/17645/10/be/src/service/impala-server.cc File be/src/service/impala-server.cc: http://gerrit.cloudera.org:8080/#/c/17645/10/be/src/service/impala-server.cc@2149 PS10, Line 2149: : nit: add a space after ":" http://gerrit.cloudera.org:8080/#/c/17645/10/be/src/service/impala-server.cc@2149 PS10, Line 2149: cur_service_id We need PrintId(cur_service_id) to get human-readable id. Same for other id loggings. http://gerrit.cloudera.org:8080/#/c/17645/10/be/src/service/impala-server.cc@2150 PS10, Line 2150: .ID in response: nit: add a space after "." and ":" http://gerrit.cloudera.org:8080/#/c/17645/10/be/src/service/impala-server.cc@2167 PS10, Line 2167: : nit: add a space after each ":" http://gerrit.cloudera.org:8080/#/c/17645/10/be/src/service/impala-server.cc@2168 PS10, Line 2168: ". The expected catalog service ID:" : << catalog_service_id << nit: we can remove this if it's logging the same id. http://gerrit.cloudera.org:8080/#/c/17645/10/tests/custom_cluster/test_restart_services.py File tests/custom_cluster/test_restart_services.py: http://gerrit.cloudera.org:8080/#/c/17645/10/tests/custom_cluster/test_restart_services.py@186 PS10, Line 186: self.execute_query_expect_success(self.client, : "alter table join_aa add columns (name string)") Could you add another test with this being executed with sync_ddl=true? Just want to make sure the logics of WaitForCatalogUpdateTopicPropagation works normally. http://gerrit.cloudera.org:8080/#/c/17645/10/tests/custom_cluster/test_restart_services.py@208 PS10, Line 208: if i == 5: : self.cluster.catalogd.restart() : if i == 6: : self.cluster.catalogd.restart() I ran this test several times but I'm not able to see the WARNING log of "Ignoring catalog update result of catalog service ID". Could you verify that this cover that branch? The log file of custom cluster test is /tmp/impalad.INFO. Maybe we just need to merge this two if-clause into one. So catalogd is restarted twice quickly. http://gerrit.cloudera.org:8080/#/c/17645/10/tests/custom_cluster/test_restart_services.py@223 PS10, Line 223: test_restart_local_catalog nit: rename to "test_restart_catalogd_with_local_catalog" -- To view, visit http://gerrit.cloudera.org:8080/17645 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9fe25f5a2a42fb432e306ef08ae35750c8f3c50c Gerrit-Change-Number: 17645 Gerrit-PatchSet: 10 Gerrit-Owner: liuyao <[email protected]> Gerrit-Reviewer: Aman Sinha <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Thomas Tauber-Marshall <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Reviewer: Vihang Karajgaonkar <[email protected]> Gerrit-Reviewer: Wenzhe Zhou <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Reviewer: liuyao <[email protected]> Gerrit-Comment-Date: Mon, 26 Jul 2021 03:00:57 +0000 Gerrit-HasComments: Yes
