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 34: Code-Review+2 (5 comments) Resolved Riza's comments with the exception of a few (see explanation in comments). Carrying +2 from Riza http://gerrit.cloudera.org:8080/#/c/22527/32/tests/beeswax/impala_beeswax.py File tests/beeswax/impala_beeswax.py: http://gerrit.cloudera.org:8080/#/c/22527/32/tests/beeswax/impala_beeswax.py@122 PS32, Line 122: 21000 > nit: Use DEFAULT_BEESWAX_PORT from common/impala_cluster.py this led to a circular dependency - IMO these default ports should be moved to another place that has less dependencies than impala_cluster.py, but replacing all imports in tests would be invasive, so I would rather not do it in this commit http://gerrit.cloudera.org:8080/#/c/22527/32/tests/common/impala_service.py File tests/common/impala_service.py: http://gerrit.cloudera.org:8080/#/c/22527/32/tests/common/impala_service.py@270 PS32, Line 270: webserver_port=25000, beeswax_port=21000, krpc_port=27000, hs2_port=21050, : hs2_http_port=28000 > nit: Use constants from common/impala_cluster.py? this led to circular dependency, see my comment in impala_beeswax.py http://gerrit.cloudera.org:8080/#/c/22527/32/tests/common/impala_test_suite.py File tests/common/impala_test_suite.py: http://gerrit.cloudera.org:8080/#/c/22527/32/tests/common/impala_test_suite.py@270 PS32, Line 270: 1st impalad > 1st impalad or None if nothing is started. Done http://gerrit.cloudera.org:8080/#/c/22527/32/tests/common/impala_test_suite.py@1800 PS32, Line 1800: l protocols. > Is this necessary? check_connections() never called with different paramete I think that it is useful to make future changes clearer. check_connections() could be useful in other tests too that need to connect to impala in non default ways, and on those cased not all clients may work. http://gerrit.cloudera.org:8080/#/c/22527/32/tests/metadata/test_event_processing_base.py File tests/metadata/test_event_processing_base.py: http://gerrit.cloudera.org:8080/#/c/22527/32/tests/metadata/test_event_processing_base.py@34 PS32, Line 34: def _run_test_insert_events_impl(cls, suite, unique_database, is_transactional=False): > Please file a follow up JIRA to clean this up based on our prior discussion created IMPALA-14174 -- 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: 34 Gerrit-Owner: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Abhishek Rawat <ara...@cloudera.com> Gerrit-Reviewer: Ashwani Raina <araina....@gmail.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Kurt Deschler <kdesc...@cloudera.com> Gerrit-Reviewer: Michael Smith <michael.sm...@cloudera.com> Gerrit-Reviewer: Riza Suminto <riza.sumi...@cloudera.com> Gerrit-Reviewer: gaurav singh <gsi...@cloudera.com> Gerrit-Comment-Date: Sat, 21 Jun 2025 07:56:19 +0000 Gerrit-HasComments: Yes
