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

Add support for preserving source message timestamp #687

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

sanjay24
Copy link

This feature allows Brooklin to make use of Kafka message's source timestamp when sending the
event to destination cluster. If the mirrored topic is configured with message.timestamp.type
set to CreateTime, the intent is to have the timestamp supplied by the application. This feature
will help achieving this intention.

This feature allows Brooklin to make use of Kafka message's source timestamp when sending the
event to destination cluster. If the mirrored topic is configured with message.timestamp.type
set to CreateTime, the intent is to have the timestamp supplied by the application. This feature
will help achieving this intention.
@sanjay24
Copy link
Author

@celiakung @ahmedahamid could you please review this PR?

@ahmedahamid
Copy link
Collaborator

@sanjay24 Will do.

@sanjay24
Copy link
Author

sanjay24 commented Mar 3, 2020

thanks @ahmedahamid @somandal

@mateuszmrozewski
Copy link

What is the expected timeline for to review this change @ahmedahamid, @somandal? It would be a very useful feature.

@ahmedahamid ahmedahamid self-requested a review March 12, 2020 22:33
@ahmedahamid
Copy link
Collaborator

Will look into it by end of next week. @mateuszmrozewski

celiakung
celiakung previously approved these changes Mar 30, 2020
Added the following test-cases:
- When preserveEventSourceTimestamp is True
- When preserveEventSourceTimestamp is False
- When preserveEventSourceTimestamp is not configured
somandal
somandal previously approved these changes Apr 2, 2020
Copy link
Collaborator

@somandal somandal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, thank you!

@sanjay24
Copy link
Author

sanjay24 commented Apr 3, 2020

@celiakung could you please check & approve the changes? Thanks

@sanjay24
Copy link
Author

sanjay24 commented Apr 8, 2020

There is still some more work required on this. I'll get back with my discovery and changes

…enabled

Current implementation passes event's source timestamp only when its timestamp type is
LogAppendTime, else it passes event's read time. This breaks the features.
This commit changes the following:
- Use event's source timestamp when the feature is enabled
- Resort to default behavior when feature is disabled
- Added associated test cases
@sanjay24
Copy link
Author

@ahmedahamid @celiakung @somandal KafkaMirrorMakerConnectorTask was translating the records to pass the source event's timestamp only when its timestamp type was "LogAppendTime". Do you know why was it done?
I've updated the PR to pass the correct timestamp when the feature is enabled.

@sanjay24
Copy link
Author

@ahmedahamid could we get moving on the review? Also, when is the next Brooklin release planned?

@sanjay24
Copy link
Author

@ahmedahamid Pinging you to get this reviewed and move ahead on this!

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.

None yet

5 participants