Minor: Fix system test broken by change of consumer group tool output…#1039
Minor: Fix system test broken by change of consumer group tool output…#1039gwenshap wants to merge 2 commits into
Conversation
|
|
||
| if group: | ||
| wait_until(lambda: ("%s, topic-consumer-group-command, 0," % group) in self.kafka.describe_consumer_group(group=group, node=kafka_node, new_consumer=enable_new_consumer, command_config=command_config_file), timeout_sec=10, | ||
| wait_until(lambda: ("%s topic-consumer-group-command 0" % group) in self.kafka.describe_consumer_group(group=group, node=kafka_node, new_consumer=enable_new_consumer, command_config=command_config_file), timeout_sec=10, |
There was a problem hiding this comment.
This seems pretty brittle -- it's totally tied to spacing choices made in the tool. At a minimum setup_and_verify isn't actually generic -- it depends on the length of group if group is not None. Actually, I'm not even clear on why the original PR changed the formatting? I get that the formatting might be nicer for display, but it seems a lot harder to parse, and I know some other tools keep things parseable (e.g. perf tools). On the other hand, others seem to prefer optimizing for human readability (e.g. kafka-topics).
There's a JIRA somewhere for providing something like --json options across all tools, and maybe that's the best long term solution, but this fix seems likely to break if anyone tweaks the output, so maybe just converting to a regex to match the relevant whitespace will be good enough for now?
|
Good point. Replaced with regexp. Regarding the need for change. The old consumer group tool used "nice" formatting, the new tool used csv - which is easier to parse but harder to read. |
|
LGTM, thanks! |
* AKCORE-22: Fix test failures (2/N)
2 participants
… format