Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/21573 )
Change subject: IMPALA-13208: Add cluster id to the membership and request-queue topic names ...................................................................... Patch Set 2: (7 comments) > Did you miss adding a custom_cluster file? Yeah, sorry about that. Forgot to add tests/custom_cluster/test_shared_catalogd.py. http://gerrit.cloudera.org:8080/#/c/21573/1/be/src/scheduling/admission-controller.h File be/src/scheduling/admission-controller.h: http://gerrit.cloudera.org:8080/#/c/21573/1/be/src/scheduling/admission-controller.h@915 PS1, Line 915: std::string request_queue_topic_name_; > I'm surprised this works without 'std::'. We tend to avoid relying on 'usin Yeah, maybe a header file explicitly declares "std::". We use "string" without "std::" in other places in this file, e.g. L811. Anyway, changed this to "std::string" to be consistent. http://gerrit.cloudera.org:8080/#/c/21573/1/be/src/scheduling/admission-controller.cc File be/src/scheduling/admission-controller.cc: http://gerrit.cloudera.org:8080/#/c/21573/1/be/src/scheduling/admission-controller.cc@670 PS1, Line 670: request_queue_topic_name_ = > line too long (96 > 90) Done http://gerrit.cloudera.org:8080/#/c/21573/1/be/src/scheduling/cluster-membership-mgr.h File be/src/scheduling/cluster-membership-mgr.h: http://gerrit.cloudera.org:8080/#/c/21573/1/be/src/scheduling/cluster-membership-mgr.h@289 PS1, Line 289: std::string membership_topic_name_; > nit: use 'std::string' Done http://gerrit.cloudera.org:8080/#/c/21573/1/be/src/statestore/statestore.cc File be/src/statestore/statestore.cc: http://gerrit.cloudera.org:8080/#/c/21573/1/be/src/statestore/statestore.cc@1235 PS1, Line 1235: return ends_with(topic, IMPALA_MEMBERSHIP_TOPIC) > line too long (99 > 90) Done http://gerrit.cloudera.org:8080/#/c/21573/1/bin/start-impala-cluster.py File bin/start-impala-cluster.py: http://gerrit.cloudera.org:8080/#/c/21573/1/bin/start-impala-cluster.py@93 PS1, Line 93: help="Start additional impalad processes.") > Can you expand on how this differs from --add-executors? --add-executors just adds executors, not coordinators: https://github.com/apache/impala/blob/c53987480726b114e0c3537c71297df2834a4962/bin/start-impala-cluster.py#L1119-L1121 So I add a new option to add abitratry impalads. But I need a hack at L1151 to set expected_num_ready_impalads. I think we'd better refactor this to have a formal option for adding a new cluster using the existing catalogd and statestore. http://gerrit.cloudera.org:8080/#/c/21573/1/tests/common/custom_cluster_test_suite.py File tests/common/custom_cluster_test_suite.py: http://gerrit.cloudera.org:8080/#/c/21573/1/tests/common/custom_cluster_test_suite.py@312 PS1, Line 312: add_impalads=False, > I don't see this option used anywhere yet. It's used in the new test that I forgot to upload. http://gerrit.cloudera.org:8080/#/c/21573/1/tests/common/impala_test_suite.py File tests/common/impala_test_suite.py: http://gerrit.cloudera.org:8080/#/c/21573/1/tests/common/impala_test_suite.py@323 PS1, Line 323: host_port = host_ports[0] > Why was this addition necessary? IMPALAD_HOST_PORT_LIST just have 3 items. When we start more impalads, we need to calculate the ports based on the first item. This is used in the new test that I forgot to upload. Added it in PS2. -- To view, visit http://gerrit.cloudera.org:8080/21573 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2ff41539f568ef03c0ee2284762b4116b313d90f Gerrit-Change-Number: 21573 Gerrit-PatchSet: 2 Gerrit-Owner: Quanlong Huang <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Comment-Date: Fri, 12 Jul 2024 00:42:38 +0000 Gerrit-HasComments: Yes
