awslabs / eventbridge-kafka-connector

Kafka sink connector for Amazon EventBridge to send events (records) from Kafka topic(s) to the specified EventBridge event bus

https://aws.amazon.com/eventbridge/resources

Apache License 2.0

61 stars 5 forks source link

Support custom `TimeMapper` class #367

Closed jensur77 closed 2 months ago

jensur77 commented 3 months ago

TimeMapper interface for setting AWS Event time

Description

Before this change the time field of the PutEventRequest was always null in which case AWS EventBridge will set it to current timestamp. This PR provides a TimeMapper interface and an optional parameter for overriding the default implementation class name. TimeMapper maps SinkRecord to Instant.

Also made Json operations more available by moving methods from DefaultEventBridgeMapper to public methods in JsonMapper.

Test Steps

Added unit tests that verify that 1. the time field is still null with no TimeMapper override 2. with TimeMapper override the time value is changed.

Checklist:

[x] I have tested my changes. No regression in existing tests.
[x] I have modified and/or added unit-tests to cover the code changes in this Pull Request.

Related Issue

358

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

jensur77 commented 2 months ago

Thanks for the review! I think i have addressed all comments now.

embano1 commented 2 months ago

@maschnetwork for final review. I unresolved a bunch of comments for Max to provide feedback.

embano1 commented 2 months ago

@jensur77 license headers are missing: https://github.com/awslabs/eventbridge-kafka-connector/actions/runs/10449849071/job/28935972672?pr=367

jensur77 commented 2 months ago

Good points. Also changed the TestTimeMapper a bit so that it makes more sense.

embano1 commented 2 months ago

@jensur77 looks like we're getting there :) Nice job! Can you please clean up your commits and create a final commit (assuming no further changes from review)?

maschnetwork commented 2 months ago

Added one final comment to the SinkRecordJsonMapper class around method and field visibility. If we can resolve this we are good to merge.

maschnetwork commented 2 months ago

@jensur77 Great - thanks for adding this. PR looks good. Before we can merge we need to resolve the two remaining issues of the build pipeline. Can you please ensure the formatting is correct with mvn com.spotify.fmt:fmt-maven-plugin:check and sqash your commits to something like feat: Added TimeMapper Interface and Default Implementation?

embano1 commented 2 months ago

@jensur77 please squash your commits into a single commit and then we're ready to 🚀

jensur77 commented 2 months ago

@embano1 Do you know if there is there a tool in github for squash? Or should I git rebase to the latest commit before we forked off?

embano1 commented 2 months ago

Github unfortunately doesn't provide an option to squash and create merge commits. I described the manual steps here: https://www.mgasch.com/2021/05/git-basics/#please-squash-your-commits (I prefer option 2)

jensur77 commented 2 months ago

@embano1 Should be one single commit now, does it look better? (rebase)