[issue-12] Add ability to set broker properties. #13
Conversation
| /** | ||
| * Defines overridden broker properties. | ||
| */ | ||
| private final Properties overrideBrokerProperties = new Properties(); |
There was a problem hiding this comment.
Why not just call this brokerProperties?
There was a problem hiding this comment.
I thought it was more descriptive, since you do not have provide a complete configuration, just whatever properties you want to set with a fall back to the libraries defaults.
Thoughts?
| * @param localHostname What IP or hostname to advertise services on. | ||
| * @param overrideBrokerProperties Define Kafka broker properties. | ||
| */ | ||
| public KafkaTestServer(final String localHostname, final Properties overrideBrokerProperties) { |
There was a problem hiding this comment.
I kind of feel like you should pull hostname from the supplied properties rather than take this in as a parameter.
There was a problem hiding this comment.
Done! To avoid breaking backwards compatibility I've left this constructor, but marked it deprecated.
| kafkaProperties.setProperty("advertised.listeners", "PLAINTEXT://" + localHostname + ":" + kafkaPort); | ||
| kafkaProperties.setProperty("listeners", "PLAINTEXT://" + localHostname + ":" + kafkaPort); | ||
| setPropertyIfNotSet(brokerProperties, "host.name", localHostname); | ||
| setPropertyIfNotSet(brokerProperties, "advertised.host.name", localHostname); |
There was a problem hiding this comment.
I wonder if you should get the hostname property you set one lime above for this.
| setPropertyIfNotSet(brokerProperties, "listeners", "PLAINTEXT://" + localHostname + ":" + kafkaPort); | ||
|
|
||
| // Set other defaults if not defined. | ||
| setPropertyIfNotSet(brokerProperties, "auto.create.topics.enable", "true"); |
There was a problem hiding this comment.
Ewww, is this legit? Never notices that before. Ditto the numbers.
| final Properties overrideProperties = new Properties(); | ||
| overrideProperties.put("broker.id", expectedBrokerId); | ||
|
|
||
|
|
| @@ -6,4 +6,4 @@ | |||
| .settings/ | |||
| target/ | |||
| README.md.bak | |||
|
|
|||
| *.versionsBackup | |||
There was a problem hiding this comment.
The maven plugin that updates release versions creates backups of POM files with this extension. Just avoiding having those checked in by accident.
…ty to set broker properties
kafka-junit4/README.md
Outdated
| @@ -84,7 +84,7 @@ Include this library in your project's POM with test scope. **You'll also need | |||
| <dependency> | |||
| <groupId>com.salesforce.kafka.test</groupId> | |||
| <artifactId>kafka-junit4</artifactId> | |||
| <version>2.2.0</version> | |||
| <version>2.2.1</version> | |||
There was a problem hiding this comment.
shouldn't it be 2.3.0? since your change isn't a bugfix but a backwards-compatible functionality
kafka-junit4/pom.xml
Outdated
| @@ -32,13 +32,13 @@ | |||
| <parent> | |||
| <artifactId>kafka-junit</artifactId> | |||
| <groupId>com.salesforce.kafka.test</groupId> | |||
| <version>2.2.0</version> | |||
| <version>2.2.1</version> | |||
There was a problem hiding this comment.
same as previous comment related to versioning.
| logger.info("Shutting down zookeeper test server"); | ||
|
|
||
| // If we don't have an instance | ||
| if (zookeeperTestServer == null) { |
There was a problem hiding this comment.
I'd probably have chosen and if-else statement plus a try-catch-finally
| } | ||
|
|
||
| // null out reference | ||
| zookeeperTestServer = null; |
There was a problem hiding this comment.
Not entirely. As the code stands we start the instance when we create the instance by telling the constructor to start. We could re-use the instance and just call start on it if it exists.
|
OK, I feel good about this. I could probably spend more time DRY'ing out all these duplicated tests and what not, but I'll save that for another day. Thanks for your help reviewing! |
for #12
Issue-12 Added ability to pass broker properties to be used by test kafka service instance.
Added helper method getAdminClient() on KafkaTestServer to get a configured AdminClient instance.
Deprecated Kafka-JUnit5 @ExtendWith annotation implementations. This has been replaced in favor of @RegisterExtension annotation. Review README.md for more information on updated usage instructions.
Deprecated KafkaTestServer constructor:
public KafkaTestServer(final String localHostname)This constructor was replaced with the constructor
KafkaTestServer(final Properties overrideBrokerProperties)where overrideBrokerProperties should contain the propertyhost.nameset to the hostname or IP address Kafka should use.