You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2020/08/22 12:57:50 UTC
[GitHub] [kafka] ivandasch commented on a change in pull request #9196: [WIP] KAFKA-10402: Upgrade system tests to python3
ivandasch commented on a change in pull request #9196:
URL: https://github.com/apache/kafka/pull/9196#discussion_r475086470
##########
File path: tests/kafkatest/benchmarks/core/benchmark_test.py
##########
@@ -88,7 +88,7 @@ def test_producer_throughput(self, acks, topic, num_producers=1, message_size=DE
self.validate_versions(client_version, broker_version)
self.start_kafka(security_protocol, security_protocol, broker_version, tls_version)
# Always generate the same total amount of data
- nrecords = int(self.target_data_size / message_size)
+ nrecords = int(self.target_data_size // message_size)
Review comment:
No need to explicit conversion to int (result is already int)
##########
File path: tests/kafkatest/services/verifiable_consumer.py
##########
@@ -372,7 +372,7 @@ def current_position(self, tp):
def owner(self, tp):
with self.lock:
- for handler in self.event_handlers.itervalues():
+ for handler in iter(self.event_handlers.values()):
Review comment:
No need to create iterator from iterable
##########
File path: tests/kafkatest/tests/client/client_compatibility_features_test.py
##########
@@ -86,14 +88,14 @@ def invoke_compatibility_program(self, features):
"--topic %s " % (self.zk.path.script("kafka-run-class.sh", node),
self.kafka.bootstrap_servers(),
len(self.kafka.nodes),
- self.topics.keys()[0]))
- for k, v in features.iteritems():
+ [*self.topics.keys()][0]))
+ for k, v in iter(features.items()):
Review comment:
No need to create iterator from iterable
##########
File path: tests/kafkatest/tests/connect/connect_distributed_test.py
##########
@@ -420,11 +421,14 @@ def test_bounce(self, clean, connect_protocol):
src_seqnos = [msg['seqno'] for msg in src_messages if msg['task'] == task]
# Every seqno up to the largest one we ever saw should appear. Each seqno should only appear once because clean
# bouncing should commit on rebalance.
- src_seqno_max = max(src_seqnos)
+ if len(src_seqnos) == 0:
+ src_seqno_max = 0
+ else:
+ src_seqno_max = max(src_seqnos)
self.logger.debug("Max source seqno: %d", src_seqno_max)
src_seqno_counts = Counter(src_seqnos)
missing_src_seqnos = sorted(set(range(src_seqno_max)).difference(set(src_seqnos)))
- duplicate_src_seqnos = sorted([seqno for seqno,count in src_seqno_counts.iteritems() if count > 1])
+ duplicate_src_seqnos = sorted([seqno for seqno,count in iter(src_seqno_counts.items()) if count > 1])
Review comment:
No need to create list and iterator, just
`sorted(seqno for seqno, count in src_seqno_counts.items() if count > 1)`
##########
File path: tests/kafkatest/services/performance/producer_performance.py
##########
@@ -78,7 +78,7 @@ def start_cmd(self, node):
'bootstrap_servers': self.kafka.bootstrap_servers(self.security_config.security_protocol),
'client_id': self.client_id,
'kafka_run_class': self.path.script("kafka-run-class.sh", node),
- 'metrics_props': ' '.join(["%s=%s" % (k, v) for k, v in self.http_metrics_client_configs.iteritems()])
+ 'metrics_props': ' '.join(["%s=%s" % (k, v) for k, v in iter(self.http_metrics_client_configs.items())])
Review comment:
No need to create iterator from items(), join can accept generator easily, no need to convert it to list.
##########
File path: tests/kafkatest/services/monitor/http.py
##########
@@ -114,7 +114,7 @@ def metrics(self, host=None, client_id=None, name=None, group=None, tags=None):
Get any collected metrics that match the specified parameters, yielding each as a tuple of
(key, [<timestamp, value>, ...]) values.
"""
- for k, values in self._http_metrics.iteritems():
+ for k, values in iter(self._http_metrics.items()):
Review comment:
redundant conversion to iterator (.items() is already iterable)
##########
File path: tests/kafkatest/services/security/security_config.py
##########
@@ -362,7 +362,7 @@ def props(self, prefix=''):
return ""
if self.has_sasl and not self.static_jaas_conf and 'sasl.jaas.config' not in self.properties:
raise Exception("JAAS configuration property has not yet been initialized")
- config_lines = (prefix + key + "=" + value for key, value in self.properties.iteritems())
+ config_lines = (prefix + key + "=" + value for key, value in iter(self.properties.items()))
Review comment:
No need to create iterator from iterable
##########
File path: tests/kafkatest/services/verifiable_consumer.py
##########
@@ -361,7 +361,7 @@ def clean_node(self, node):
def current_assignment(self):
with self.lock:
- return { handler.node: handler.current_assignment() for handler in self.event_handlers.itervalues() }
+ return { handler.node: handler.current_assignment() for handler in iter(self.event_handlers.values()) }
Review comment:
No need to create iterator from iterable
##########
File path: tests/kafkatest/services/monitor/http.py
##########
@@ -154,7 +154,7 @@ def do_POST(self):
name = raw_metric['name']
group = raw_metric['group']
# Convert to tuple of pairs because dicts & lists are unhashable
- tags = tuple([(k, v) for k, v in raw_metric['tags'].iteritems()]),
+ tags = tuple([(k, v) for k, v in iter(raw_metric['tags'].items())]),
Review comment:
Same as above + tuple can be constructed from generator, no need to create list for that
##########
File path: tests/kafkatest/services/verifiable_consumer.py
##########
@@ -386,33 +386,33 @@ def last_commit(self, tp):
def total_consumed(self):
with self.lock:
- return sum(handler.total_consumed for handler in self.event_handlers.itervalues())
+ return sum(handler.total_consumed for handler in iter(self.event_handlers.values()))
Review comment:
Same as above
##########
File path: tests/kafkatest/services/verifiable_consumer.py
##########
@@ -386,33 +386,33 @@ def last_commit(self, tp):
def total_consumed(self):
with self.lock:
- return sum(handler.total_consumed for handler in self.event_handlers.itervalues())
+ return sum(handler.total_consumed for handler in iter(self.event_handlers.values()))
def num_rebalances(self):
with self.lock:
- return max(handler.assigned_count for handler in self.event_handlers.itervalues())
+ return max(handler.assigned_count for handler in iter(self.event_handlers.values()))
def num_revokes_for_alive(self, keep_alive=1):
with self.lock:
- return max([handler.revoked_count for handler in self.event_handlers.itervalues()
+ return max([handler.revoked_count for handler in iter(self.event_handlers.values())
Review comment:
Same as above
##########
File path: tests/kafkatest/services/verifiable_consumer.py
##########
@@ -386,33 +386,33 @@ def last_commit(self, tp):
def total_consumed(self):
with self.lock:
- return sum(handler.total_consumed for handler in self.event_handlers.itervalues())
+ return sum(handler.total_consumed for handler in iter(self.event_handlers.values()))
def num_rebalances(self):
with self.lock:
- return max(handler.assigned_count for handler in self.event_handlers.itervalues())
+ return max(handler.assigned_count for handler in iter(self.event_handlers.values()))
Review comment:
Same as above.
##########
File path: tests/kafkatest/services/verifiable_consumer.py
##########
@@ -386,33 +386,33 @@ def last_commit(self, tp):
def total_consumed(self):
with self.lock:
- return sum(handler.total_consumed for handler in self.event_handlers.itervalues())
+ return sum(handler.total_consumed for handler in iter(self.event_handlers.values()))
def num_rebalances(self):
with self.lock:
- return max(handler.assigned_count for handler in self.event_handlers.itervalues())
+ return max(handler.assigned_count for handler in iter(self.event_handlers.values()))
def num_revokes_for_alive(self, keep_alive=1):
with self.lock:
- return max([handler.revoked_count for handler in self.event_handlers.itervalues()
+ return max([handler.revoked_count for handler in iter(self.event_handlers.values())
if handler.idx <= keep_alive])
def joined_nodes(self):
with self.lock:
- return [handler.node for handler in self.event_handlers.itervalues()
+ return [handler.node for handler in iter(self.event_handlers.values())
if handler.state == ConsumerState.Joined]
def rebalancing_nodes(self):
with self.lock:
- return [handler.node for handler in self.event_handlers.itervalues()
+ return [handler.node for handler in iter(self.event_handlers.values())
if handler.state == ConsumerState.Rebalancing]
def dead_nodes(self):
with self.lock:
- return [handler.node for handler in self.event_handlers.itervalues()
+ return [handler.node for handler in iter(self.event_handlers.values())
Review comment:
Same as above
##########
File path: tests/kafkatest/services/verifiable_consumer.py
##########
@@ -386,33 +386,33 @@ def last_commit(self, tp):
def total_consumed(self):
with self.lock:
- return sum(handler.total_consumed for handler in self.event_handlers.itervalues())
+ return sum(handler.total_consumed for handler in iter(self.event_handlers.values()))
def num_rebalances(self):
with self.lock:
- return max(handler.assigned_count for handler in self.event_handlers.itervalues())
+ return max(handler.assigned_count for handler in iter(self.event_handlers.values()))
def num_revokes_for_alive(self, keep_alive=1):
with self.lock:
- return max([handler.revoked_count for handler in self.event_handlers.itervalues()
+ return max([handler.revoked_count for handler in iter(self.event_handlers.values())
if handler.idx <= keep_alive])
def joined_nodes(self):
with self.lock:
- return [handler.node for handler in self.event_handlers.itervalues()
+ return [handler.node for handler in iter(self.event_handlers.values())
if handler.state == ConsumerState.Joined]
def rebalancing_nodes(self):
with self.lock:
- return [handler.node for handler in self.event_handlers.itervalues()
+ return [handler.node for handler in iter(self.event_handlers.values())
Review comment:
Same as above
##########
File path: tests/kafkatest/services/verifiable_consumer.py
##########
@@ -386,33 +386,33 @@ def last_commit(self, tp):
def total_consumed(self):
with self.lock:
- return sum(handler.total_consumed for handler in self.event_handlers.itervalues())
+ return sum(handler.total_consumed for handler in iter(self.event_handlers.values()))
def num_rebalances(self):
with self.lock:
- return max(handler.assigned_count for handler in self.event_handlers.itervalues())
+ return max(handler.assigned_count for handler in iter(self.event_handlers.values()))
def num_revokes_for_alive(self, keep_alive=1):
with self.lock:
- return max([handler.revoked_count for handler in self.event_handlers.itervalues()
+ return max([handler.revoked_count for handler in iter(self.event_handlers.values())
if handler.idx <= keep_alive])
def joined_nodes(self):
with self.lock:
- return [handler.node for handler in self.event_handlers.itervalues()
+ return [handler.node for handler in iter(self.event_handlers.values())
if handler.state == ConsumerState.Joined]
def rebalancing_nodes(self):
with self.lock:
- return [handler.node for handler in self.event_handlers.itervalues()
+ return [handler.node for handler in iter(self.event_handlers.values())
if handler.state == ConsumerState.Rebalancing]
def dead_nodes(self):
with self.lock:
- return [handler.node for handler in self.event_handlers.itervalues()
+ return [handler.node for handler in iter(self.event_handlers.values())
if handler.state == ConsumerState.Dead]
def alive_nodes(self):
with self.lock:
- return [handler.node for handler in self.event_handlers.itervalues()
+ return [handler.node for handler in iter(self.event_handlers.values())
Review comment:
Same as above
##########
File path: tests/kafkatest/tests/client/client_compatibility_features_test.py
##########
@@ -86,14 +88,14 @@ def invoke_compatibility_program(self, features):
"--topic %s " % (self.zk.path.script("kafka-run-class.sh", node),
self.kafka.bootstrap_servers(),
len(self.kafka.nodes),
- self.topics.keys()[0]))
- for k, v in features.iteritems():
+ [*self.topics.keys()][0]))
Review comment:
I suppose that next(iter(self.topics.keys())) is more readable, than this mix of brackets and stars
##########
File path: tests/kafkatest/tests/connect/connect_distributed_test.py
##########
@@ -420,11 +421,14 @@ def test_bounce(self, clean, connect_protocol):
src_seqnos = [msg['seqno'] for msg in src_messages if msg['task'] == task]
# Every seqno up to the largest one we ever saw should appear. Each seqno should only appear once because clean
# bouncing should commit on rebalance.
- src_seqno_max = max(src_seqnos)
+ if len(src_seqnos) == 0:
+ src_seqno_max = 0
+ else:
+ src_seqno_max = max(src_seqnos)
Review comment:
`src_seqno_max = max(src_seqnos) if len(src_seqnos) else 0` is more readable
##########
File path: tests/kafkatest/version.py
##########
@@ -49,6 +49,34 @@ def __str__(self):
else:
return LooseVersion.__str__(self)
+ def __eq__(self, other):
Review comment:
If you start to refactor this class, may be refactor also call to super constructor? LooseVersion is now normal class (in python3). Just `super().__init__(version_string)`
##########
File path: tests/kafkatest/tests/end_to_end.py
##########
@@ -87,7 +85,7 @@ def on_record_consumed(self, record, node):
def await_consumed_offsets(self, last_acked_offsets, timeout_sec):
def has_finished_consuming():
- for partition, offset in last_acked_offsets.iteritems():
+ for partition, offset in iter(last_acked_offsets.items()):
Review comment:
No need to create iterator
##########
File path: tests/kafkatest/tests/verifiable_consumer_test.py
##########
@@ -40,7 +40,7 @@ def _all_partitions(self, topic, num_partitions):
def _partitions(self, assignment):
partitions = []
- for parts in assignment.itervalues():
+ for parts in iter(assignment.values()):
Review comment:
No need to create iterator
##########
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 = [*self.topics.keys()][topic_index]
Review comment:
`list(self.topics.keys())[topic_index]` is more readable
##########
File path: tests/kafkatest/tests/client/quota_test.py
##########
@@ -162,7 +162,7 @@ def test_quota(self, quota_type, override_quota=True, producer_num=1, consumer_n
jmx_attributes=['bytes-consumed-rate'], version=client_version)
consumer.run()
- for idx, messages in consumer.messages_consumed.iteritems():
+ for idx, messages in iter(consumer.messages_consumed.items()):
Review comment:
No need to create iterator
----------------------------------------------------------------
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