Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/22527 )
Change subject: IMPALA-13820: add ipv6 support for webui/hs2/hs2-http/beeswax ...................................................................... Patch Set 23: (6 comments) http://gerrit.cloudera.org:8080/#/c/22527/23/be/src/rpc/thrift-server.h File be/src/rpc/thrift-server.h: http://gerrit.cloudera.org:8080/#/c/22527/23/be/src/rpc/thrift-server.h@348 PS23, Line 348: The host name to with nit: The host name to bind with? http://gerrit.cloudera.org:8080/#/c/22527/23/shell/impala_shell/impala_client.py File shell/impala_shell/impala_client.py: http://gerrit.cloudera.org:8080/#/c/22527/23/shell/impala_shell/impala_client.py@432 PS23, Line 432: host = "[%s]" % self.impalad_host if ":" in self.impalad_host else self.impalad_host : host_and_port = "{0}:{1}".format(host, self.impalad_port) Please change this to a function so that we can leave comment about IPv6 handling. Can __parse_host_port reused here? http://gerrit.cloudera.org:8080/#/c/22527/23/tests/beeswax/impala_beeswax.py File tests/beeswax/impala_beeswax.py: http://gerrit.cloudera.org:8080/#/c/22527/23/tests/beeswax/impala_beeswax.py@172 PS23, Line 172: if self.transport and self.connected: : self.transport.close() Why need to check for self.connected? I thought self.transport.close() is idempotent, regardless of connected or not? http://gerrit.cloudera.org:8080/#/c/22527/23/tests/common/impala_service.py File tests/common/impala_service.py: http://gerrit.cloudera.org:8080/#/c/22527/23/tests/common/impala_service.py@73 PS23, Line 73: is_ipv6 = ":" in self.webserver_interface : host = "[%s]" % self.webserver_interface if is_ipv6 else self.webserver_interface Can this be a static function in common/network.py ? http://gerrit.cloudera.org:8080/#/c/22527/23/tests/common/impala_test_suite.py File tests/common/impala_test_suite.py: http://gerrit.cloudera.org:8080/#/c/22527/23/tests/common/impala_test_suite.py@505 PS23, Line 505: @classmethod Can this be removed? All reference to ImpalaTestSuite.create_impala_client are replaced with self.create_impala_client. http://gerrit.cloudera.org:8080/#/c/22527/23/tests/metadata/test_event_processing_base.py File tests/metadata/test_event_processing_base.py: http://gerrit.cloudera.org:8080/#/c/22527/23/tests/metadata/test_event_processing_base.py@33 PS23, Line 33: @classmethod : def _run_test_insert_events_impl(cls, suite, unique_database, is_transactional=False): Why this need to be classmethod instead of regular class member that can be called through self._run_test_insert_events_impl by child class? Same question for _run_event_based_replication_tests_impl. -- 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: 23 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: Riza Suminto <[email protected]> Gerrit-Reviewer: gaurav singh <[email protected]> Gerrit-Comment-Date: Tue, 27 May 2025 16:00:14 +0000 Gerrit-HasComments: Yes
