KAFKA-2431: Easier Testing of SSL by benstopford · Pull Request #217 · apache/kafka

benstopford · 2015-09-15T02:12:42Z

Allow cipher suites to be specified relevant properties
Avoid System.exit in ProducerPerformance so this can be externally invoked
Add command line option so that a default properties can be specified in ConsumerPerformance (needed for ssl properties)
Allow cipher suites to be specified via properties
Increase timeout in ConsumerPerformance (useful when running with SSL under load)
Use a relative default value for the reporting interval in Consumer Performance
Add option for show-all-stats (includes intermediary and summary stats) in Consumer Performance

eribeiro · 2015-09-15T02:36:16Z

nit: space between if and (.

Also, I would suggest to do if (!cipherSuitesList.isEmpty()) 😉

asfbot · 2015-09-15T17:13:15Z

kafka-trunk-git-pr #428 FAILURE
Looks like there's a problem with this pull request

asfbot · 2015-09-15T18:03:54Z

kafka-trunk-git-pr #429 SUCCESS
This pull request looks good

ijuma · 2015-09-16T12:08:16Z

Out of curiosity, why are we doing the isEmpty check here? We don't do the same check when setting the enabledProtocols. It seems to add noise for little benefit given how this is used.

It's because the enabled protocols have a kafka supplied default value. With the cipher suite I left it up to the JVM.

Oh I see, if you pass an empty collection, the JDK defaults are no longer used? The change makes sense then. We should probably be using the JDK defaults for enabled protocols by default too IMO, but that can be handled via KAFKA-2456

Yes - I agree. I don't think overriding the JVM defaults is a good idea unless there is a specific, documented intent.

ijuma · 2015-09-16T12:32:27Z

A few mostly minor comments, the rest looks good. The command-line switch option name is something I think is important to address before merging.

asfbot · 2015-09-19T16:50:11Z

kafka-trunk-git-pr #450 SUCCESS
This pull request looks good

ijuma · 2015-09-20T01:29:11Z

LGTM

ijuma · 2015-09-22T10:37:10Z

@harshach, are you interested in reviewing and merging this PR since you are now a committer? /cc @gwenshap @junrao too

asfbot · 2015-09-24T07:46:46Z

kafka-trunk-git-pr #515 SUCCESS
This pull request looks good

asfbot · 2015-09-24T08:34:20Z

kafka-trunk-git-pr #516 SUCCESS
This pull request looks good

asfbot · 2015-09-24T09:06:22Z

kafka-trunk-git-pr #517 SUCCESS
This pull request looks good

asfbot · 2015-09-24T09:07:07Z

kafka-trunk-git-pr #518 FAILURE
Looks like there's a problem with this pull request

asfbot · 2015-09-24T09:09:25Z

kafka-trunk-git-pr #519 FAILURE
Looks like there's a problem with this pull request

asfbot · 2015-09-24T09:41:42Z

kafka-trunk-git-pr #520 FAILURE
Looks like there's a problem with this pull request

asfbot · 2015-09-24T09:55:10Z

kafka-trunk-git-pr #521 FAILURE
Looks like there's a problem with this pull request

asfbot · 2015-09-24T10:01:07Z

kafka-trunk-git-pr #522 FAILURE
Looks like there's a problem with this pull request

asfbot · 2015-09-24T10:06:48Z

kafka-trunk-git-pr #523 FAILURE
Looks like there's a problem with this pull request

benstopford · 2015-09-24T11:58:09Z

tests run locally

ijuma · 2015-09-24T11:59:43Z

Do we really need a new option for this? I was a bit surprised that showDetailedStats doesn't show the final result to be honest. You probably did this for compatibility reasons, but it would be good to get a second opinion from @gwenshap or @junrao.

I know exactly what you mean fella. In fact my first reaction was to just change the behaviour, but it's an external interface so people may have coupled to this behaviour. That's why I went down the additional option route.

Yes, I understand. It seems like we don't offer strong compatibility guarantees for these scripts though, see discussion in #242. Do you think that people would actually rely on the fact that there is no additional entry with the overall results at the end?

Quite likely not I expect, but we should overhaul these scripts if we're going down that route. I should add that the way we use jopt a little problematic. We end up overriding the system's default values just because jopt leads you down the line of providing a default value. Lots of the values used don't make much sense. For settings that map directly to kafka properties it would be better if we enforced that mapping explicitly.

I agree with @ijuma . It seems it's simpler to just always print the final result instead of introducing a new option. This is a change of behavior, but it probably matches most people's expectation. ProducerPerformance is already like that.

benstopford · 2015-09-29T18:08:35Z

Addressed comments from above. Also added a few other bits:

The ability to pass freeform properties of the format key=val,key=val etc. Generally useful but avoids the need for a properties file for SSL
Added a header that prints the properties being used (so it's clear what is being overridden as various defaults are hard coded in both ProducerPerformance and ConsumerPerformance and differ from documented defaults)
Made linger.ms a command line option for ProducerPerf. This is an important setting.

asfbot · 2015-09-29T18:12:32Z

kafka-trunk-git-pr #599 FAILURE
Looks like there's a problem with this pull request

ijuma · 2015-09-29T18:13:03Z

Note that this doesn't merge cleanly anymore.

ijuma · 2015-09-29T18:13:56Z

Space after if

asfbot · 2015-09-29T18:51:53Z

kafka-trunk-git-pr #600 FAILURE
Looks like there's a problem with this pull request

- Avoid System.exit in ProducerPerformance so this can be externally invoked - Add command line option so that a default set of properties can be speicfied in ConsumerPerformance (needed for ssl properties) - Allow cipher suites to be specified via properties - Increase timeout in ConsumerPerformance (useful when running with SSL under load) - Use a relative default value for the reporting interval - Add option for show-all-stats (includes intermediary and summary stats)

benstopford · 2015-09-29T18:56:56Z

Addressed comments from @ijuma addressed. Rebased.

asfbot · 2015-09-29T18:59:26Z

kafka-trunk-git-pr #601 FAILURE
Looks like there's a problem with this pull request

- paramaterised test-timeout in the consumer so it's a command line option - added header for Producer/Consumer properties (so it's clear what is being overridden) - added command line option to Producer/Consumer for freeform properties of the format key=val,key=val etc - made linger.ms a command line option for ProducerPerf. This is a key setting. - cleanup based on comments from Jun/Ismael

asfbot · 2015-09-29T19:38:08Z

kafka-trunk-git-pr #602 FAILURE
Looks like there's a problem with this pull request

asfbot · 2015-09-29T19:58:58Z

kafka-trunk-git-pr #603 FAILURE
Looks like there's a problem with this pull request

asfbot · 2015-09-29T21:36:21Z

kafka-trunk-git-pr #609 SUCCESS
This pull request looks good

ijuma · 2015-10-02T17:42:42Z

Just noticed that ConsoleProducer has the following:

val producerPropertyOpt = parser.accepts("producer-property", "A mechanism to pass user-defined properties in the form key=value to the producer. ") .withRequiredArg .describedAs("producer_prop") .ofType(classOf[String])

And it uses CommandLineUtils.parseKeyValueArgs(options.valuesOf(propertyOpt)). We probably should be using the same naming and parsing code here.

…ests (apache#217) TICKET = LIKAFKA-41598 LI_DESCRIPTION = This extends the existing integration-test framework by adding a "sister tree" of test-harness classes modified for multi-cluster use. (For now, the client side still supports only cluster 0.) EXIT_CRITERIA = N/A

…ests (apache#305) The original commit in 2.4-li branch is - [LI-HOTFIX] Basic server-side support for multi-cluster integration tests (apache#217) TICKET = LIKAFKA-41598 LI_DESCRIPTION = This extends the existing integration-test framework by adding a "sister tree" of test-harness classes modified for multi-cluster use. (For now, the client side still supports only cluster 0.) EXIT_CRITERIA = N/A Co-authored-by: Greg Roelofs <newt@pobox.com>

…pache#217) This reverts commit a100f00, reversing changes made to a035a5f.

Simplify LME lookup to direct failback only

eribeiro reviewed Sep 15, 2015
View reviewed changes

benstopford force-pushed the ssl-1 branch from 42dfaaa to 18fb3f0 Compare September 15, 2015 17:02

ijuma reviewed Sep 16, 2015
View reviewed changes

benstopford force-pushed the ssl-1 branch from bcd2a7c to 037d566 Compare September 24, 2015 07:24

benstopford force-pushed the ssl-1 branch from d1b3f1c to da89353 Compare September 24, 2015 09:51

benstopford force-pushed the ssl-1 branch from da89353 to ada34ef Compare September 24, 2015 09:55

ijuma reviewed Sep 24, 2015
View reviewed changes

ijuma reviewed Sep 29, 2015
View reviewed changes

Comment thread core/src/main/scala/kafka/tools/ConsumerPerformance.scala Outdated

ijuma Sep 29, 2015

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.

Space after if

benstopford force-pushed the ssl-1 branch from 2dc20d3 to 923b585 Compare September 29, 2015 18:54

benstopford force-pushed the ssl-1 branch from e9afb47 to 81a1f2e Compare September 29, 2015 19:09

KAFKA-2431: added missing license files

149ee43

ijuma reviewed Oct 2, 2015
View reviewed changes

benstopford closed this Oct 9, 2015

benstopford deleted the ssl-1 branch October 22, 2015 11:56

efeg pushed a commit to efeg/kafka that referenced this pull request Jan 29, 2020

Fix a concurrency issue in MetricSampleAggregator (apache#217)

c2791d9

davide-armand pushed a commit to aiven/kafka that referenced this pull request Dec 1, 2025

Revert "Merge pull request apache#208 from aiven/jeqo/ink-148-fix" (a…

43d122b

…pache#217) This reverts commit a100f00, reversing changes made to a035a5f.

jeqo added a commit to aiven/kafka that referenced this pull request Jan 16, 2026

Revert "Merge pull request apache#208 from aiven/jeqo/ink-148-fix" (a…

5cccc2d

…pache#217) This reverts commit a100f00, reversing changes made to a035a5f.

fvaleri pushed a commit to fvaleri/kafka that referenced this pull request Jul 8, 2026

Merge pull request apache#217 from fvaleri/lme-lookup-2

b812673

Simplify LME lookup to direct failback only

Uh oh!

Conversation

benstopford commented Sep 15, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asfbot commented Sep 15, 2015

Uh oh!

asfbot commented Sep 15, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ijuma commented Sep 16, 2015

Uh oh!

asfbot commented Sep 19, 2015

Uh oh!

ijuma commented Sep 20, 2015

Uh oh!

ijuma commented Sep 22, 2015

Uh oh!

asfbot commented Sep 24, 2015

Uh oh!

asfbot commented Sep 24, 2015

Uh oh!

asfbot commented Sep 24, 2015

Uh oh!

asfbot commented Sep 24, 2015

Uh oh!

asfbot commented Sep 24, 2015

Uh oh!

asfbot commented Sep 24, 2015

Uh oh!

asfbot commented Sep 24, 2015

Uh oh!

asfbot commented Sep 24, 2015

Uh oh!

asfbot commented Sep 24, 2015

Uh oh!

benstopford commented Sep 24, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benstopford commented Sep 29, 2015

Uh oh!

asfbot commented Sep 29, 2015

Uh oh!

ijuma commented Sep 29, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asfbot commented Sep 29, 2015

Uh oh!

benstopford commented Sep 29, 2015

Uh oh!

asfbot commented Sep 29, 2015

Uh oh!

asfbot commented Sep 29, 2015

Uh oh!

asfbot commented Sep 29, 2015

Uh oh!

asfbot commented Sep 29, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees