[GitHub] [kafka] edenhill commented on a change in pull request #9196: KAFKA-10402: Upgrade system tests to python3

[GitBox <gi...@apache.org>]

edenhill commented on a change in pull request #9196:
URL: https://github.com/apache/kafka/pull/9196#discussion_r482798753



##########
File path: tests/docker/Dockerfile
##########
@@ -32,9 +32,11 @@ ARG ducker_creator=default
 LABEL ducker.creator=$ducker_creator
 
 # Update Linux and install necessary utilities.
-RUN apt update && apt install -y sudo netcat iptables rsync unzip wget curl jq coreutils openssh-server net-tools vim python-pip python-dev libffi-dev libssl-dev cmake pkg-config libfuse-dev iperf traceroute && apt-get -y clean
-RUN python -m pip install -U pip==9.0.3;
-RUN pip install --upgrade cffi virtualenv pyasn1 boto3 pycrypto pywinrm ipaddress enum34 && pip install --upgrade ducktape==0.7.9
+RUN apt-mark hold python2 python2-minimal python2.7 python2.7-minimal libpython2-stdlib libpython2.7-minimal libpython2.7-stdlib

Review comment:
       Add a comment explaining why the hold is necssary.

##########
File path: tests/docker/Dockerfile
##########
@@ -32,9 +32,11 @@ ARG ducker_creator=default
 LABEL ducker.creator=$ducker_creator
 
 # Update Linux and install necessary utilities.
-RUN apt update && apt install -y sudo netcat iptables rsync unzip wget curl jq coreutils openssh-server net-tools vim python-pip python-dev libffi-dev libssl-dev cmake pkg-config libfuse-dev iperf traceroute && apt-get -y clean
-RUN python -m pip install -U pip==9.0.3;
-RUN pip install --upgrade cffi virtualenv pyasn1 boto3 pycrypto pywinrm ipaddress enum34 && pip install --upgrade ducktape==0.7.9
+RUN apt-mark hold python2 python2-minimal python2.7 python2.7-minimal libpython2-stdlib libpython2.7-minimal libpython2.7-stdlib
+RUN apt update && apt install -y sudo netcat iptables rsync unzip wget curl jq coreutils openssh-server net-tools vim python3-pip python3-dev libffi-dev libssl-dev cmake pkg-config libfuse-dev iperf traceroute mc && apt-get -y clean
+RUN python3 -m pip install -U pip==20.2.2;
+RUN pip3 install --upgrade cffi virtualenv pyasn1 boto3 pycrypto pywinrm ipaddress enum34
+RUN pip3 install git+https://github.com/confluentinc/ducktape

Review comment:
       Should ducktape no longer be version pinned? (it probably needs to be to avoid future build breakages of old kafka branches).
   Or is this just during Python3-ification of ducktape itself?

##########
File path: tests/kafkatest/tests/streams/streams_broker_bounce_test.py
##########
@@ -119,7 +119,7 @@ def __init__(self, test_context):
     def fail_broker_type(self, failure_mode, broker_type):
         # Pick a random topic and bounce it's leader
         topic_index = randint(0, len(self.topics.keys()) - 1)
-        topic = self.topics.keys()[topic_index]
+        topic = list(self.topics.keys())[topic_index]

Review comment:
       alternatively:
   `topic = random.choice(list(self.topics.keys()))`

##########
File path: tests/kafkatest/tests/core/replica_scale_test.py
##########
@@ -48,7 +46,7 @@ def teardown(self):
         self.zk.stop()
 
     @cluster(num_nodes=12)
-    @parametrize(topic_count=500, partition_count=34, replication_factor=3)
+    @parametrize(topic_count=100, partition_count=34, replication_factor=3)

Review comment:
       Since this PR is about upgrading to Python3 it probably shouldn't modify test parameters.

##########
File path: tests/kafkatest/tests/core/network_degrade_test.py
##########
@@ -129,10 +129,10 @@ def test_rate(self, task_name, device_name, latency_ms, rate_limit_kbit):
         self.logger.info("Measured rates: %s" % measured_rates)
 
         # We expect to see measured rates within an order of magnitude of our target rate
-        low_kbps = rate_limit_kbit / 10
+        low_kbps = rate_limit_kbit // 10
         high_kbps = rate_limit_kbit * 10
         acceptable_rates = [r for r in measured_rates if low_kbps < r < high_kbps]
 
         msg = "Expected most of the measured rates to be within an order of magnitude of target %d." % rate_limit_kbit
-        msg += " This means `tc` did not limit the bandwidth as expected."
+        msg += " This means `tc` did not limit the bandwidth as expected. Measured rates %s" % str(measured_rates)

Review comment:
       nit: I believe % is a bit deprecated in favour of `.format(..)` or `f"This means .. {measured_rates}"` (for >=3.6).




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org