Csaba Ringhofer 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 24: (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 bind > nit: The host name to bind with? Done 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_and_port = self._to_host_port(self.impalad_host, self.impalad_port) : assert self.http_path > Please change this to a function so that we can leave comment about IPv6 ha Done 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? It isn't idempotent for impyla + SSL, see my change in ImpalaTestSuite.close_impala_clients() This may be considered a bug in the connection, but generally I don't think that we should assume that a connection can closed twice. 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: host_port = to_host_port(self.webserver_interface, self.webserver_port) : url = "%s://%s/%s" % (protocol, host_port, page_name) > Can this be a static function in common/network.py ? Done 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? The problem is that this function and create_impala_client() are called in setup_class(cls), so it cannot be an instence method. Probably the clearest would be to make these global functions that use global variables like the_cluster, as it is something inherently global. 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 I agree that these shouldn't be class methods. I think that the original reason is that TestEventProcessing and TestEventProcessingError do not inherit from TestEventProcessingBase in this patch. It is is easy to fix this, but it is not a small change due to the lot of changes from "cls" to "self". I would prefer to clean this up in another patch. Btw I found a test issue (IMPALA-14109), TestEventProcessing and TestEventProcessingError are actually skipped based on the previous test. -- 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: 24 Gerrit-Owner: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Abhishek Rawat <[email protected]> Gerrit-Reviewer: Ashwani Raina <[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: Wed, 28 May 2025 15:17:37 +0000 Gerrit-HasComments: Yes
