Skip to content

MINOR: Pass KafkaVersion to nodes in KafkaService. Related to PR#7000#7025

Closed
rhauch wants to merge 1 commit intoapache:trunkfrom
rhauch:fix-kafka-service-version
Closed

MINOR: Pass KafkaVersion to nodes in KafkaService. Related to PR#7000#7025
rhauch wants to merge 1 commit intoapache:trunkfrom
rhauch:fix-kafka-service-version

Conversation

@rhauch
Copy link
Copy Markdown
Contributor

@rhauch rhauch commented Jul 2, 2019

PR #7000 added a KafkaVersion.support_named_listeners() method and used this in tests/kafkatest/services/kafka/templates/kafka.properties. However, if KafkaService expects the version parameter to be a string (as denoted in the doc), then KafkaService needs to construct a KafkaVersion and pass it to the nodes rather than passing the string, since the string does not have the support_named_listeners() method.

This could be an alternative to #7023.

Committer Checklist (excluded from commit message)

  • Verify design and implementation
  • Verify test coverage and CI build status
  • Verify documentation (including upgrade notes)

@rhauch
Copy link
Copy Markdown
Contributor Author

rhauch commented Jul 2, 2019

System tests build with this change: https://jenkins.confluent.io/job/system-test-kafka-branch-builder/2765/

Copy link
Copy Markdown
Contributor

@rajinisivaram rajinisivaram left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rhauch Thanks for the PR, the doc for KafkaService wrongly describes version as String. All callers except the failing connect tests pass in KafkaVersion rather than a String. So I think we should update these instead:

@parametrize(broker_version=str(DEV_BRANCH), auto_create_topics=False, security_protocol=SecurityConfig.PLAINTEXT, connect_protocol='compatible')
. Left a note to this effect in #7023.

@rhauch
Copy link
Copy Markdown
Contributor Author

rhauch commented Jul 3, 2019

Thanks, Rajini. Closing without merging.

@rhauch rhauch closed this Jul 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants