kafka: extend client.id parsing per producer/consumer #426
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jeqo
force-pushed
the
kafka-client-id-per-prod-cons
branch
from
0a00002 to
874db8a
Compare
|
|
||
| When running workers, pass the `zone.id`: | ||
|
|
||
| ```bash |
| /opt/benchmark/bin/benchmark-worker | ||
| ``` | ||
|
|
||
| Then pass the `client.id` template: |
There was a problem hiding this comment.
Why not use something like envsubst
| private Properties topicProperties; | ||
| private Properties producerProperties; | ||
| private Properties consumerProperties; | ||
| // Visible for testing |
There was a problem hiding this comment.
I'd use a Hashmap/table + enum for the three types
| @@ -76,6 +77,14 @@ public void initialize(File configurationFile, StatsLogger statsLogger) throws I | |||
| producerProperties = new Properties(); | |||
| commonProperties.forEach((key, value) -> producerProperties.put(key, value)); | |||
| @@ -76,6 +77,14 @@ public void initialize(File configurationFile, StatsLogger statsLogger) throws I | |||
| producerProperties = new Properties(); | |||
| commonProperties.forEach((key, value) -> producerProperties.put(key, value)); | |||
| producerProperties.load(new StringReader(config.producerConfig)); | |||
|
|
|||
| if (producerProperties.containsKey(KAFKA_CLIENT_ID)) { | |||
| producerProperties.put( | |||
There was a problem hiding this comment.
Isn't there a putIfAbsent inversion?
|
|
||
| try (KafkaBenchmarkDriver driver = new KafkaBenchmarkDriver()) { | ||
| // When initializing kafka driver | ||
| Files.write(configPath, KafkaBenchmarkDriver.mapper.writeValueAsBytes(config)); |
There was a problem hiding this comment.
Use a ByteArray Stream rather than a file
|
|
||
| // Then | ||
| if (producerClientId != null) { | ||
| assertThat(driver.producerProperties).containsEntry("client.id", producerClientId); |
There was a problem hiding this comment.
Assert against CommonConfig constants not magic string
| driver.initialize(configPath.toFile(), null); | ||
|
|
||
| // Then | ||
| if (producerClientId != null) { |
There was a problem hiding this comment.
Nit: Optional types are better than nullable Objects
| } else { | ||
| assertThat(driver.producerProperties).doesNotContainKey("client.id"); | ||
| } | ||
| if (consumerClientId != null) { |
There was a problem hiding this comment.
You should assert nullability based on parameterized test order , otherwise this is just lazy testing
| assertThat(driver.producerProperties).doesNotContainKey("client.id"); | ||
| } | ||
| if (consumerClientId != null) { | ||
| assertThat(driver.consumerProperties).containsEntry("client.id", consumerClientId); |
There was a problem hiding this comment.
I assume containsEntry does the key check short circuit?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
For further flexibility, it may be useful to differentiate client ids when passing a zone id.
Currently, if a zone id variable is needed, it needs to be in common configurations, forcing both producer and consumer to have the same values (with different zones).
This proposal extends the same parsing to client ids from producer and consumers; adds some unit tests; and documents this feature.