Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

KAFKA-5623: ducktape kafka service: do not assume Service contains nu… #3557

Closed
wants to merge 1 commit into from

Conversation

cmccabe
Copy link
Contributor

@cmccabe cmccabe commented Jul 21, 2017

…m_nodes

Copy link
Contributor

@ewencp ewencp left a comment

Choose a reason for hiding this comment

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

One nit, but changes look reasonable. Have we also verified this works with v0.6.0? I can't imagine how it'd break anything, but would be good to be sure.

@@ -191,7 +192,7 @@ def prop_file(self, node):
# TODO - clean up duplicate configuration logic
prop_file = cfg.render()
prop_file += self.render('kafka.properties', node=node, broker_id=self.idx(node),
security_config=self.security_config)
security_config=self.security_config, num_nodes=self.num_nodes)
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, is this required? Normally render should automatically have access to all fields of the object it's being called on. I'm not sure why the security_config one is there either.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it's not required. I'll remove it.

@cmccabe
Copy link
Contributor Author

cmccabe commented Jul 21, 2017

Yes, I tested with ducktape 0.6.0 as well as ducktape master

@asfgit
Copy link

asfgit commented Jul 21, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.11/6229/
Test PASSed (JDK 7 and Scala 2.11).

@asfgit
Copy link

asfgit commented Jul 21, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/6213/
Test PASSed (JDK 8 and Scala 2.12).

Copy link
Contributor

@ewencp ewencp left a comment

Choose a reason for hiding this comment

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

@cmccabe LGTM, thanks for sorting this out!

asfgit pushed a commit that referenced this pull request Jul 21, 2017
…m_nodes

…m_nodes

Author: Colin P. Mccabe <cmccabe@confluent.io>

Reviewers: Ewen Cheslack-Postava <ewen@confluent.io>

Closes #3557 from cmccabe/KAFKA-5623

(cherry picked from commit 9ae09e8)
Signed-off-by: Ewen Cheslack-Postava <me@ewencp.org>
asfgit pushed a commit that referenced this pull request Jul 21, 2017
…m_nodes

…m_nodes

Author: Colin P. Mccabe <cmccabe@confluent.io>

Reviewers: Ewen Cheslack-Postava <ewen@confluent.io>

Closes #3557 from cmccabe/KAFKA-5623

(cherry picked from commit 9ae09e8)
Signed-off-by: Ewen Cheslack-Postava <me@ewencp.org>
@asfgit asfgit closed this in 9ae09e8 Jul 21, 2017
@cmccabe cmccabe deleted the KAFKA-5623 branch May 20, 2019 18:44
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.

3 participants