Skip to content

Fix for a failing upgrade test#7000

Merged
rajinisivaram merged 1 commit intoapache:trunkfrom
stan-is-hate:check-failing-test
Jun 27, 2019
Merged

Fix for a failing upgrade test#7000
rajinisivaram merged 1 commit intoapache:trunkfrom
stan-is-hate:check-failing-test

Conversation

@stan-is-hate
Copy link
Copy Markdown
Contributor

@stan-is-hate stan-is-hate commented Jun 26, 2019

Previous PR (#6938) added partial support for named listeners to kafka.py. Though we did run full test suite, somehow we missed one of the upgrade tests.
Since upgrade tests start kafka from older versions, they may try to start it from a version that doesn't support named listeners yet.
This PR addresses the issue by adding a method to version.py called supports_named_listeners() and calling that method from kafka.properties template.
According to KIP-103 (KAFKA-4636), inter.broker.listener.name property was introduced in 0.10.2.0, so I used this value in supports_named_listeners() method.

Testing:
Verified that a previously failing test passes locally.
Ran upgrade tests on Jenkins - https://jenkins.confluent.io/job/system-test-kafka-branch-builder/2747, all green

Committer Checklist (excluded from commit message)

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

….protocol and inter.broker.listener.name depending on kafka version
@stan-is-hate stan-is-hate marked this pull request as ready for review June 26, 2019 16:48
@stan-is-hate
Copy link
Copy Markdown
Contributor Author

@rajinisivaram @brianbushree @ijuma can you please help review this PR? Please let me know if we need a full test run for it

@stan-is-hate
Copy link
Copy Markdown
Contributor Author

@mumrah

@brianbushree
Copy link
Copy Markdown
Contributor

if these kafka versions don't support named listeners, can we still use use_separate_interbroker_listener parameter? will that have no effect? or what will happen exactly?

@stan-is-hate
Copy link
Copy Markdown
Contributor Author

@brianbushree good question.
If kafka version doesn't support named listeners, this param should be ignored really. Let me make the change.

@stan-is-hate
Copy link
Copy Markdown
Contributor Author

@brianbushree I suggest the following change in kafka.py:

    def get_lowest_version(self):
        return min(node.version for node in self.nodes)

    def setup_interbroker_listener(self, security_protocol, use_separate_listener=False):
        self.use_separate_interbroker_listener = self.get_lowest_version().supports_named_listeners \
                                                 and use_separate_listener
        ....

This way the only way people can get into the state where their node is running a version that doesn't support named listeners (< 0.10.2.0) but at the same time kafka is using a named listener (use_separate_interbroker_listener == True) is if they set use_separate_interbroker_listener=True in constructor and after that call smth like self.kafka.nodes[0].version = V_0_9_2_0. We cannot really hook into a setter for node.version but I don't think it will be a huge issue - I would expect people setting different versions on different nodes to know what they're doing.
What do you think?

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.

@stan-confluent Thanks for the PR, LGTM

@brianbushree
Copy link
Copy Markdown
Contributor

@stan-confluent makes sense, LGTM!

@rajinisivaram
Copy link
Copy Markdown
Contributor

@stan-confluent Thanks for the PR, merging to trunk.

@rajinisivaram rajinisivaram merged commit 594d043 into apache:trunk Jun 27, 2019
rajinisivaram pushed a commit to confluentinc/kafka that referenced this pull request Jun 27, 2019
…ker.protocol and inter.broker.listener.name depending on kafka version (apache#7000)

Reviewers: Brian Bushree <bbushree@confluent.io>, Rajini Sivaram <rajinisivaram@googlemail.com>
rajinisivaram pushed a commit to confluentinc/kafka that referenced this pull request Jun 27, 2019
…ker.protocol and inter.broker.listener.name depending on kafka version (apache#7000)

Reviewers: Brian Bushree <bbushree@confluent.io>, Rajini Sivaram <rajinisivaram@googlemail.com>
Copy link
Copy Markdown
Contributor

@hachikuji hachikuji left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for the patch!

hachikuji pushed a commit that referenced this pull request May 27, 2020
…ker.protocol and inter.broker.listener.name depending on kafka version (#7000)

Reviewers: Brian Bushree <bbushree@confluent.io>, Rajini Sivaram <rajinisivaram@googlemail.com>
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.

4 participants