Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/22527 )
Change subject: WIP: add ipv6 support for webui/hs2/hs2-http/beeswax ...................................................................... Patch Set 16: (8 comments) http://gerrit.cloudera.org:8080/#/c/22527/8//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/22527/8//COMMIT_MSG@25 PS8, Line 25: bin/start-impala-cluster.py --impalad_args="--external_interface=:: --webserver_interface=::" --catalogd_args="--webserver_interface=::" --state_store_args="--webserver_interface=::" > Please try to simplify the arguments so IPV6 is easier to configure. Having I was thinking about adding an argument to start-impala-cluster.py like --shared_arg="" which would be appended to the startup flags of all daemons (+ a similar thing could be done in custom cluster tests). Do you think that this is a good direction? http://gerrit.cloudera.org:8080/#/c/22527/14/be/src/util/network-util.cc File be/src/util/network-util.cc: http://gerrit.cloudera.org:8080/#/c/22527/14/be/src/util/network-util.cc@88 PS14, Line 88: char addr_buf[INET6_ADDRSTRLEN + 1]; > INET6_ADDRSTRLEN + 1 Done http://gerrit.cloudera.org:8080/#/c/22527/14/be/src/util/network-util.cc@246 PS14, Line 246: ss << "[" << address.hostname << "]:" << dec << address.port; > Maybe also check if the hostname already has "[" and "]" ? I think that we shouldn't see the [] form here, that should only appear in urls http://gerrit.cloudera.org:8080/#/c/22527/14/be/src/util/webserver.cc File be/src/util/webserver.cc: http://gerrit.cloudera.org:8080/#/c/22527/14/be/src/util/webserver.cc@415 PS14, Line 415: listening_spec_v6 << "[" << ip << "]:" << http_address_.port; > Maybe also check if the hostname already has "[" and "]" ? I don't think that brackets should be possible in the interface name, but I may be wrong. http://gerrit.cloudera.org:8080/#/c/22527/14/shell/impala_shell.py File shell/impala_shell.py: http://gerrit.cloudera.org:8080/#/c/22527/14/shell/impala_shell.py@977 PS14, Line 977: def __parse_host_port(addr): : """Checks if the host name also contains a port and separates the two. : Returns either [host] or [host, port]. Detects if host is an ipv6 address like "[::]" : and removes the brackets from it. : """ : split_by_colon = addr.split(':') : ipv6addr = len(split_by_colon) > 2 : host_port = None : if ipv6addr: : if addr[0] == "[": : parts = addr.split("]") : host_port = [parts[0][1:]] : has_port = parts[1] != "" : if has_port: : if parts[1][0] != ":": return None : host_port.append(parts[1][1:]) : else: : host_port = [addr] : else: > Add one line description (with example) for the function, what its trying t Done http://gerrit.cloudera.org:8080/#/c/22527/14/tests/common/impala_connection.py File tests/common/impala_connection.py: http://gerrit.cloudera.org:8080/#/c/22527/14/tests/common/impala_connection.py@600 PS14, Line 600: host, port = split_host_port(self.__host_port) > what happens if the input is invalid format and returns None ? I don't worry much in this case, as this is a test utility class. Can add some assert in split_host_port() if needed. http://gerrit.cloudera.org:8080/#/c/22527/14/tests/common/impala_test_suite.py File tests/common/impala_test_suite.py: http://gerrit.cloudera.org:8080/#/c/22527/14/tests/common/impala_test_suite.py@275 PS14, Line 275: ImpalaTestSuite.impalad_test_service = cls.impalad_test_service > What does this do? This is a hack to ensure that ImpalaTestSuite always impalad_test_service as static member. Without this cls is a subclass of ImpalaTestSuite then ImpalaTestSuite would not have this static member, which causes problems in utility functions that call ImpalaTestSuite static functions directly, so without the subclass available. http://gerrit.cloudera.org:8080/#/c/22527/14/tests/common/network.py File tests/common/network.py: http://gerrit.cloudera.org:8080/#/c/22527/14/tests/common/network.py@37 PS14, Line 37: def split_host_port(host_port): > Add function description. This seems to be similar to the function __parse_ Done -- To view, visit http://gerrit.cloudera.org:8080/22527 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I51ac66c568cc9bb06f4a3915db07a53c100109b6 Gerrit-Change-Number: 22527 Gerrit-PatchSet: 16 Gerrit-Owner: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Abhishek Rawat <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Kurt Deschler <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: gaurav singh <[email protected]> Gerrit-Comment-Date: Thu, 27 Mar 2025 17:20:25 +0000 Gerrit-HasComments: Yes
