Skip to content

KAFKA-2581: Run some existing ducktape tests with SSL#271

Closed
rajinisivaram wants to merge 4 commits intoapache:trunkfrom
rajinisivaram:KAFKA-2581
Closed

KAFKA-2581: Run some existing ducktape tests with SSL#271
rajinisivaram wants to merge 4 commits intoapache:trunkfrom
rajinisivaram:KAFKA-2581

Conversation

@rajinisivaram
Copy link
Copy Markdown
Contributor

Parametrize console consumer sanity test, replication tests and benchmarks tests to run with both PLAINTEXT and SSL.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should these be added before or after the command line options? If we load them before, the default values for options would override custom settings from the consumer.config file even if the command line parameters were not specified.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Interesting. Ideally, specified command-line settings would override the properties, but it seems like that would be a bigger change. Perhaps this should behave as the ConsoleConsumer until we can overhaul this. It looks like the change here does that, right?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Agreed, ideally if they were specified they would override, but if unspecified it would use values from the file. Not sure how easy that is to get working with this options library. I could have sworn I checked one of the other tools and found it using the behavior I expected, but now can't find it. I don't think this is make-or-break. I was just raising it since the behavior is unintuitive to me and although the performance tools are less critical, we probably wouldn't want to change their behavior that significantly after releasing a version with support for the consumer.config option.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I do completely agree. However, I think all the tools currently do it this way and so I have kept it consistent. I will raise another JIRA to fix all the tools.

@ewencp
Copy link
Copy Markdown
Contributor

ewencp commented Oct 5, 2015

@rajinisivaram This is looking good. I left a bunch of comments, but the core functionality that adds SSL test support seems solid. The changes I suggested are mostly targeted at code organization for the tests since the functionality seems ok.

@rajinisivaram
Copy link
Copy Markdown
Contributor Author

@ewencp @ijuma Thank you for review comments. I will work on the changes tomorrow and submit an update soon.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Minor but important - change these camel-case variables to snake_case for consistency

@rajinisivaram
Copy link
Copy Markdown
Contributor Author

Test failure is unrelated to this PR.

Have addressed most of the issues raised. I didn't used mixin for security config, but have changed the code layout as suggested by @ewencp .

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

self.test_context = test_context is not necessary

every subclass of Test has this assigned as long as you make the proper call to super(...).__init__(...) (which you did)

@granders
Copy link
Copy Markdown
Contributor

granders commented Oct 8, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I discussed this offline with @GuoZhang - to avoid having empty dummy files floating around, it may be better to delete this file and pass in empty string to in the SecurityConfig constructor in verifiable_producer (see comment below)

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.

5 participants