Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding ProducerRecord to ConsumerRecord mapper to MockKafka builder #2

Merged
merged 11 commits into from
Jan 24, 2020

Conversation

sivanmz
Copy link
Contributor

@sivanmz sivanmz commented Jan 23, 2020

The current implementation discards the record headers. The constructors for ConsumerRecord have some arguments that don't make sense for mocking, but I do need the headers.

I thought it would be good to parameterize the mapping from ProducerRecord to ConsumerRecord, with the default being the current implementation.

(I'm not that experienced with Git and pull requests and had to clean up some accidental reformatting)

Example usage:

    _kafkaFactory = new MockKafka<String, byte[]>()
            .withRecordMapper((inProducerRecord, inRecordMetadata) -> {
                int partition = inProducerRecord.partition() != null ? inProducerRecord.partition() : inRecordMetadata.partition();
                return new ConsumerRecord<String, byte[]>(inProducerRecord.topic(), partition, inRecordMetadata.offset(),
                        inRecordMetadata.timestamp(),
                        TimestampType.NO_TIMESTAMP_TYPE, -1L, -1, -1, inProducerRecord.key(),
                        inProducerRecord.value(), inProducerRecord.headers());
            });

…lt mapper does not include metadata and headers. Alternative mappers can now be specified.
…he default mapper does not include metadata and headers. Alternative mappers can now be specified."

Reapplied without previous reformatting.

This reverts commit 0589e41.
…nsumerRecord mapper. The default mapper does not include metadata and headers. Alternative mappers can now be specified.""

Removed reformatting. Reapplied changes.

This reverts commit bf4135f
…rd to ConsumerRecord mapper. The default mapper does not include metadata and headers. Alternative mappers can now be specified."""

This reverts commit 7d4a94c
…he default mapper does not include metadata and headers. Alternative mappers can now be specified."

This reverts commit 0589e41
…per. The default mapper does not include metadata and headers. Alternative mappers can now be specified. No reformatting.
@codecov-io
Copy link

codecov-io commented Jan 23, 2020

Codecov Report

Merging #2 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##             master     #2   +/-   ##
=======================================
  Coverage       100%   100%           
- Complexity      256    259    +3     
=======================================
  Files            42     42           
  Lines           724    730    +6     
  Branches         47     47           
=======================================
+ Hits            724    730    +6
Impacted Files Coverage Δ Complexity Δ
...n/java/com/obsidiandynamics/jackdaw/MockKafka.java 100% <100%> (ø) 32 <8> (+3) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b37c366...d57a70e. Read the comment docs.

@ekoutanov
Copy link
Member

Would you mind fixing the coverage issue? I would like to run it at 100% branch coverage please.

Also, I'd happily have us change the default mapper implementation to push the headers through. Makes sense to do it.

@sivanmz
Copy link
Contributor Author

sivanmz commented Jan 24, 2020

Added test. Something is reformatting my commits, I can try to fix if you prefer. Sorry.

@ekoutanov
Copy link
Member

Nah that's ok. What do you think of making header passing as part of the default mapper?

@sivanmz
Copy link
Contributor Author

sivanmz commented Jan 24, 2020

Agree that headers should be passed by default.

@ekoutanov
Copy link
Member

If you'd like to make that change, I'll pull it all in one go.

@sivanmz
Copy link
Contributor Author

sivanmz commented Jan 24, 2020

Done

@ekoutanov ekoutanov merged commit ac45cc7 into obsidiandynamics:master Jan 24, 2020
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.

3 participants