Trogdor: Added commonClientConf and adminClientConf to workload specs by apovzner · Pull Request #4757 · apache/kafka

apovzner · 2018-03-22T17:02:26Z

Currently, WorkerUtils will be able to create topics when there is no security. To be able to work with secure kafka, WorkerUtils.createTopic() needs to be able to take security configs. This PR adds commonClientConf field to both producer bench and roundtrip workload specs so that users can specify security and other common configs once for producer/consumer and adminClient. Also added adminClientConf field to workload specs so that users can specify adminClient specific configs if they want to. For completeness, added consumerConf and producerConf to roundtrip workload spec.

Committer Checklist (excluded from commit message)

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

ijuma · 2018-03-22T17:33:37Z

+        SslConfigs.SSL_TRUSTSTORE_LOCATION_CONFIG,
+        SslConfigs.SSL_TRUSTSTORE_PASSWORD_CONFIG,
+        SslConfigs.SSL_PROTOCOL_CONFIG,
+        SslConfigs.SSL_ENDPOINT_IDENTIFICATION_ALGORITHM_CONFIG


Can you not use .withClientSslSupport().withClientSaslSupport()?

I did not know about those methods. Let me look. Thanks!

…coding sasl and ssl related keys

cmccabe · 2018-03-22T20:28:57Z

Thanks, seems reasonable.

Maybe we should have a clientConfig field in the Spec which describes all the configuration keys which are common to all clients (admin, producer, consumer). That might be simpler than trying to maintain whitelists of configuration keys that can bleed from one config to another.

apovzner · 2018-03-22T20:43:29Z

@cmccabe I think with withClientSaslSupport().withClientSslSupport() we don't really whitelist anymore and that should work with changes in client configs. So I think pulling from producer/consumer configs is much simpler than asking people to separate common vs. producer/consumer specific configs in their specs.

cmccabe · 2018-03-23T20:02:12Z

I think it's wrong to copy configuration keys meant for the producer to the admin client or to the consumer. Although it's appealing as a quick hack that gets things to work, it quickly breaks down when things get even slightly more complicated. For example, what if someone wants to set a long request timeout for the admin client, but a short one for the producer? Perhaps the user wants to test producer timeouts, but not worry about topic creation timing out. But this is impossible with the current patch-- you haven't given the user any way to configure the AC separate from the producer.

We already have other tests such as RoundTripWorkload which have both a producer and a consumer. Are we going to use "producer config" to configure both? Or make the user repeat configurations twice? Neither of these options are appealing.

I think it's sensible to just separate configuration into common client configs, producer configs, and consumer configs. That should allow people to set whatever configurations they want, and also keep things simple.

apovzner · 2018-03-23T20:24:55Z

Wouldn't common client config have the same issue as you mentioned configuring request timeout differently say for producer and consumer? Or you mean producer or consumer configs will be able to overwrite whatever is in common client config? Plus, for your example, we will need to add admin client config to the specs.

Also, how would people know what to put to common config and what not? I think it may make things confusing for users.

Don't you think it is better in that case to add "adminClientConf' to producer and roundtrip specs? Which will mean that the user will need to specify security configs multiple times.

cmccabe · 2018-03-23T20:42:48Z

Wouldn't common client config have the same issue as you mentioned configuring request timeout differently say for producer and consumer? Or you mean producer or consumer configs will be able to overwrite whatever is in common client config?

Yes, I think the producer and consumer configs should override whatever is in the common client config.

Plus, for your example, we will need to add admin client config to the specs.

Yes, I agree. But that's easy to do. And if the user doesn't specify the admin client config, it just defaults to being empty (doing nothing)

Also, how would people know what to put to common config and what not? I think it may make things confusing for users.

The things that are in the common config should be the things that are common to all clients, like connection settings and so forth. I don't think it's that hard for users. In fact, most users probably would just use the common config for everything, until they start doing something advanced.

Don't you think it is better in that case to add "adminClientConf' to producer and roundtrip specs? Which will mean that the user will need to specify security configs multiple times.

The user won't have to specify the security configuration multiple times. As you described above, the user can just put that in the common configuration.

apovzner · 2018-03-23T22:09:24Z

@cmccabe Ok, I agree with common client config approach. I changed this PR to use "commonClientConf" in producer and roundtrip spec. Lets not add adminClientConfig yet until we actually need it? Or you think we need it now?

cmccabe · 2018-03-27T18:57:16Z

It might be easier to add adminClientConf now, just so that we can change those settings later if we want without filing a PR. I don't feel strongly about it, though, since we solved the immediate problems

apovzner · 2018-03-30T22:43:45Z

@cmccabe I added adminClientConf to both ProducerBench and RoundTripWorkload specs, and using it in WorkerUtils.

cmccabe · 2018-04-02T17:16:10Z

Thanks, @apovzner . LGTM

Maybe add producer config and consumer config to RoundTripWorkload, just to make it easy for people later to change those?

apovzner · 2018-04-03T15:59:25Z

@cmccabe I added producerConf and consumerConf to roundtrip workload spec

cmccabe · 2018-04-04T16:42:45Z

It seems like RoundTripWorker is not using common client conf to set up producer and consumer configurations?

apovzner · 2018-04-04T18:15:53Z

@cmccabe you are right, I forgot to set common client props in producer and consumer configs in roundtrip worker. Just checked in the fix.

cmccabe · 2018-04-05T16:50:04Z

Thanks, @apovzner. LGTM. We could also consider having a utility function to merge two config maps... that might be useful here

rajinisivaram

@apovzner Thanks for the PR. Left a couple of minor comments, apart from that LGTM.

rajinisivaram · 2018-04-05T23:14:18Z

    }
+
+    protected Map<String, String> configOrEmptyMap(Map<String, String> config) {
+        return (config == null) ? new TreeMap<String, String>() : config;


Could just use Collections.emptyMap()?

rajinisivaram · 2018-04-05T23:15:57Z

            props.put(ProducerConfig.CLIENT_ID_CONFIG, "producer." + id);
            props.put(ProducerConfig.ACKS_CONFIG, "all");
            props.put(ProducerConfig.REQUEST_TIMEOUT_MS_CONFIG, 105000);
+            // user may over-write the defaults with common client config and consumer config


consumer => producer?

rajinisivaram · 2018-04-06T18:19:01Z

@apovzner Thanks for the update, merging to trunk.

…apache#4757) Currently, WorkerUtils will be able to create topics when there is no security. To be able to work with secure kafka, WorkerUtils.createTopic() needs to be able to take security configs. This PR adds commonClientConf field to both producer bench and roundtrip workload specs so that users can specify security and other common configs once for producer/consumer and adminClient. Also added adminClientConf field to workload specs so that users can specify adminClient specific configs if they want to. For completeness, added consumerConf and producerConf to roundtrip workload spec. Reviewers: Ismael Juma <ismael@juma.me.uk>, Colin P. Mccabe <cmccabe@confluent.io>, Rajini Sivaram <rajinisivaram@googlemail.com>

apovzner added 2 commits March 22, 2018 09:56

Pulling client security props to AdminClient config

1d1f103

fixed checkstyle warn

6c27aaa

ijuma reviewed Mar 22, 2018

View reviewed changes

Using .withClientSaslSupport().withClientSslSupport() instead of hard…

429ecdb

…coding sasl and ssl related keys

Using commonClientConfig for common client props

c2992b1

Added adminClientConf to ProduceBench and RoundTripWorkload specs

590fb5c

apovzner changed the title ~~Trogdor: Pulling client security props to AdminClient config~~ Trogdor: Added commonClientConf and adminClientConf to workload specs Mar 30, 2018

Added producerConf and consumerConf to roundtrip workload spec

d9892a7

Using props from commonConf in roundtrip workload

3d116bf

apovzner added 2 commits April 5, 2018 14:34

Added utility method to merge configs and unit tests

decb50d

Moved configOrEmptyMap as protected method to base class

9459655

rajinisivaram approved these changes Apr 5, 2018

View reviewed changes

using Collections.emptyMap() and fixed one comment

8a53398

rajinisivaram merged commit da32db9 into apache:trunk Apr 6, 2018

apovzner deleted the adminclient-security branch April 7, 2018 01:28

Uh oh!

Conversation

apovzner commented Mar 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Committer Checklist (excluded from commit message)

Uh oh!

ijuma Mar 22, 2018

Choose a reason for hiding this comment

Uh oh!

apovzner Mar 22, 2018

Choose a reason for hiding this comment

Uh oh!

cmccabe commented Mar 22, 2018

Uh oh!

apovzner commented Mar 22, 2018

Uh oh!

cmccabe commented Mar 23, 2018

Uh oh!

apovzner commented Mar 23, 2018

Uh oh!

cmccabe commented Mar 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

apovzner commented Mar 23, 2018

Uh oh!

cmccabe commented Mar 27, 2018

Uh oh!

apovzner commented Mar 30, 2018

Uh oh!

cmccabe commented Apr 2, 2018

Uh oh!

apovzner commented Apr 3, 2018

Uh oh!

cmccabe commented Apr 4, 2018

Uh oh!

apovzner commented Apr 4, 2018

Uh oh!

cmccabe commented Apr 5, 2018

Uh oh!

rajinisivaram left a comment

Choose a reason for hiding this comment

Uh oh!

rajinisivaram Apr 5, 2018

Choose a reason for hiding this comment

Uh oh!

rajinisivaram Apr 5, 2018

Choose a reason for hiding this comment

Uh oh!

rajinisivaram commented Apr 6, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

apovzner commented Mar 22, 2018 •

edited

Loading

cmccabe commented Mar 23, 2018 •

edited

Loading